Skip to content
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

Update ivi, remove kivi #210

Merged
merged 1 commit into from
Jul 14, 2017
Merged

Update ivi, remove kivi #210

merged 1 commit into from
Jul 14, 2017

Conversation

localvoid
Copy link
Contributor

Updated ivi to 0.8.0. This update shouldn't have any impact on client-side performance, with 0.8.0 I've been focusing on SSR performance.

Removed kivi, there is almost no difference in performance between kivi and ivi, and ivi covers significantly more edge cases.

@krausest
Copy link
Owner

@localvoid I'll merge in the next few days and fix the conflict.

@krausest krausest merged commit d7dfa14 into krausest:master Jul 14, 2017
@krausest
Copy link
Owner

Thanks. Results are here

@adamhaile
Copy link
Contributor

A question on this one. I thought we had agreed not to use the rAF optimization? Looks like this commit switched ivi to put all updates behind an rAF?

@leeoniya
Copy link
Contributor

leeoniya commented Jul 16, 2017

the waters are pretty murky here :/

domvm's latest impl also removed the forced-synchronous redraw, which implicitly means it also benefits from this "optimization". however, all of domvm's redraw calls are rAF-throttled by default, so this perf is what a user would expect to see in real life without relying on obscure async dom optimization techniques and simply relying on official framework-idiomatic optimizations.

certainly i'm against making implementations more complex and less idiomatic simply to avoid this behavior if it already exists at the lib level. whether or not it's a reasonable thing to allow in impls of libs that do not support it and do not offer any kind official redraw scheduler, and only to the "remove all rows" case seems too narrow of an allowance.

it appears that here, updateNextFrame is framework-provided, runs for every action, and i assume comes without additional caveats. so certainly much more general than requestAnimationFrame for "remove all rows" only, which has all the hallmarks of micro-optimization for this specific bench.

-- my biased $0.02

@localvoid
Copy link
Contributor Author

@adamhaile I've hoped that all implementations will be implemented without rAF, but after this PR I've also changed to rAF.

@leeoniya
Copy link
Contributor

leeoniya commented Jul 17, 2017

that impl is without rAF, especially without selective rAF. i simply exposed the natural behavior of the lib by simplifying the impl. my position, i think, is consistent with the prior discussion.

@adamhaile
Copy link
Contributor

adamhaile commented Jul 17, 2017

In most of the apps I've tested, putting DOM updates after an rAF call is usually a bit slower than running them immediately. The worst I saw was a canvas-based app that had a 30% drop in FPS when I added rAF. If I remember right, Stefan found that some of the results for vanillajs suffered when he applied rAF to them as well.

All that makes me think that a) we still don't understand when or why rAF is an optimization, and b) until we do, it'll be a task-specific try-it-and-see thing. If this benchmark happened to be slower with rAF, you'd probably turn it off, right? So we're still tuning to the task.

It seems like there are two possible paths:

  1. these are petri-dish experiments in figuring out how to work the DOM as fast as we can, so all approaches are open.

  2. since we don't understand how it works and how specific it might be to this benchmark, remove rAF-tuning from the table by setting a standard policy

I'm OK with either of those. The third option I find a bit weird:

  1. it might be ok and might not be ok, depending on how syntactically elegant you do it

BTW, surplus has a similar ability to do app-wide rAF. I did it in a really specific and hacky way before because I wanted to highlight the oddity and because I figured any app could do it that way, nullifying any advantage.

[Edit: feeling like I was too harsh about option 3. I get that there's a real issue about whether we're writing apps "like a normal user" or "like the guy/gal who wrote the framework and can tune it out the wazoo." It's more than just "syntactic elegance." ¯\_(ツ)_/¯ . Won't somebody come up with a real optimization, so we can stop talking about this stuff? 😄]

@localvoid
Copy link
Contributor Author

we still don't understand when or why rAF is an optimization

In ivi it is a way to properly schedule read/write DOM ops and nothing else, for example it is ok to read from DOM in event handlers, because it will guarantee that all DOM updates will be performed when rAF is triggered.

If this benchmark happened to be slower with rAF, you'd probably turn it off, right?

Yes.

I get that there's a real issue about whether we're writing apps "like a normal user" or "like the guy/gal who wrote the framework and can tune it out the wazoo."

As you can see with ivi implementation, it doesn't use any weird "advanced optimizations" and components are encapsulated. It even intentionally implemented with connectors to show that it won't have significant impact on performance like it does for React-clones+Redux :) Optimizing library for an average code path instead of a special "fast path" is way much harder.

@ryansolid ryansolid mentioned this pull request May 27, 2018
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.

None yet

4 participants