Skip to content

Add skruv #791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 26, 2020
Merged

Add skruv #791

merged 1 commit into from
Sep 26, 2020

Conversation

SvanteRichter
Copy link
Contributor

This adds the skruv micro-framework.

I read some of the discussions around requestAnimationFrame (like in #430) which I use to schedule a render, but since my framework leaves scheduling of renders up to the user (and it renders the whole tree, not just the deleted row) that's fine, I hope. Let me know otherwise and if I should change the scheduling to setTimeout/requestIdleCallback or similar.

I did not do any build steps as that is my preferred/recommended way to use it, I'm sure some metrics would be better if I did. I might open a PR for such a benchmark in the future if that's fine.

Most of the implementation is copied from the hyperapp one since that is the other framework I've been using recently and the way to structure the views feels similar.

Results from my machine compared to hyperapp, vue, react do not show anything too far out of normal (although they are better for skruv than I thought) to indicate something being totally wrong, but that's just my guess:

Screenshot from 2020-09-22 20-58-26

It uses one "hack", but that is not really decided on how to deal with it, I have an issue for fixing/documenting in skruv/skruv#2

I'm new to both writing frameworks and contributing benchmarks, so please let me know what I can do better in the future :)

@krausest krausest added the merging started merging started (no more updates please) label Sep 26, 2020
@krausest krausest merged commit 42faa06 into krausest:master Sep 26, 2020
@krausest krausest removed the merging started merging started (no more updates please) label Sep 26, 2020
@krausest
Copy link
Owner

Thanks. Using RAF for just some operations in the benchmark should be regarded as cheating. Using RAF for all operations should be acceptable, but might be considered a code smell. I added a note to the implementation (when I find the time it should be yellow for notes and red for issues. This one would be yellow)
Here are the results on my machine:

@SvanteRichter
Copy link
Contributor Author

@krausest Thanks! What would be your suggestion to schedule a render for a bunch of different state changes that happen within a short time? I'm looking for something to only schedule one render for a bunch of state changes that happen within short intervals.

Is setTimeout with a low timeout (1-2) better for this or is RAF considered a codesmell mostly because it has potential for cheating? Or would it be better to just do the work on the next tick via setTimeout with timeout 0?

@ryansolid
Copy link
Contributor

It's an interesting area. Some frameworks schedule under the hood by default to do this batching like you described. Most commonly using a microtask (like a Promise resolution). Some use animation frames since in theory why do any updates quicker than the end-user would see.

Animation Frame is actually slower for most of these operations but improves performance on a couple. So in the past people abused this (myself included). So while it might seem overly manual to have that be the question posed to the end-user, if that's the position being taken it's consistent. It's just a unique choice to not have a default batching behavior so when you see it you immediately ask is something specific to this benchmark suite is being done.

@krausest
Copy link
Owner

@SahAssar Sorry I can't give you good advice on that except that as a user I'd rather prefer the framework to solve that problem for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants