Closed lifeart closed 5 years ago
updated keying logic
@NullVoxPopuli
While I have no idea how the internals work whenever you see disproportionately low replace rows and swap row scores comparatively it's worth checking your solution is still keyed. While sometimes it is just the inconsistency of benchmark runs, those 2 and remove row are the ones that dramatically improve in non-keyed. It's possible @index is forcing non-keyed (ie.. using the array index as the key).
Actually seeing the rest of your discussion now everything you are seeing makes complete sense. Basically there are 3 ways to attack array reconciliation. 2 of them are keyed by the definition of this benchmark and 1 is not.
Identity(Keyed) - uses referential check. This is what KVO libraries use by default generally since their data is mutable and references are fixed. This is usually the most performant keyed approach but it is incompatible with immutable data. VDOM libraries cannot use this approach generally for that reason.
Explicit Key(Keyed) - Uses key check. Necessary for Immutable renderers to maintain keyed updates. KVO libraries could use this approach but usually it's not necessary. This is part of the recommended API for most VDOM/Top Down libraries.
Index(Non-Keyed) - Uses the array index. Essentially reusing the existing nodes and just updating the values. There are performance benefits in Replace Rows, Swap Rows, and Remove Row, but it comes at the cost of losing DOM state. This is the default behavior for VDOM libraries if a key is not set. For KVO you have to go out of your way to set this mode since the default is identity.
If Ember uses that naming convention for these 3 modes that suggests that while the results are good, you are perhaps looking at incorrect comparison, comparing Non-Key Ember with Keyed implementations.
Thanks @ryansolid for chiming in!
@lifeart indeed it seems the current implementation would qualify as "ember-v3.9.1-non-keyed", but not as the keyed version. See also https://www.stefankrause.net/wp/?p=504
Still not sure why in the case of GlimmerVM the keyed version would be so much slower. When I last debugged into it, I remember that for the swap test, basically all rows were moved around, although in the most optimized case only touching those two rows that are swapped would be sufficient.
Here for the record my remarks in Ember's Discord after those findings (end of last year):
@chancancode any idea why GlimmerVM is so far behind other frameworks, especially for the swap-benchmark and the keyed version?
I don't think Ember's performance particularly worse than other libraries in certain areas but more just across the board. I mean the Swap Rows is likely a very naive approach but half libraries do that. You go up the list one by one until you find an item out of place then insert the right item at that location pushing the rest back. But you will always be off by one index so you basically bubble up the first row all the way to end. You end up moving every element between the 2 indexes. Other libraries use a different set of heuristics to pre-emptively decide to do minimal set of DOM operations since those are the expensive operations.
It is very obvious Ember takes a hit on initial row spamming since it has to set up all it's reactive data where VDOM libraries can have less of an impact. But the partial updates is where I'd be concerned. For KVO library you should be owning that. It's the most pure look at the overhead of the change management system. So that is basically your best case. If you are slow there many other benchmarks may be proportionally slow since it's the same change mechanism behind it and you aren't likely winning the row spamming race anyway. Sure using the right DOM API's are part of the puzzle here, but I imagine in many cases Ember is using similar things to other popular libraries.
@simonihmig @ryansolid PR updated to use @identity
But I'm wondering, why glimmer-vm beat this gap using identity? (see https://github.com/krausest/js-framework-benchmark/pull/555)
Shouldn't the tests check for incorrect inserts?
looks like it's an bug in glimmer-vm - ember
integration
Look at where Glimmer VM does well. I wouldn't be surprised it's non-keyed.
@ryansolid it's keyed (at least in template syntaxis level - same as ember version)
@lifeart, is this something we can make a repro repo for the glimmer folks?
Maybe it's a bug.? While preact isn't necessarily the fastest benchmark to have on the left side. If it isn't just variation in runs Replace Row is absurdly low. Most libraries are within 20% on replace rows. So when you see 57 there to React and Preacts 130s it may mean something is up. Again it's Benchmark 2, 5, and 6 that are driving the performance numbers up.
BTW - The test whether an implementation is keyed can be run with following command in webdriver-ts:
npm run isKeyed -- --framework ember-v3.5.1-keyed
(The test tries its best to distinguish between keyed and non-keyed but was outsmarted a few times in the past)
@lifeart Do you have an idea why there's no version number for react and preact in your table? As long as there's a package-lock.json in the directory for the react and preact implementation the table should show that version.
But you will always be off by one index so you basically bubble up the first row all the way to end.
Yes, this was exactly what I was observing.
Other libraries use a different set of heuristics to pre-emptively decide to do minimal set of DOM operations since those are the expensive operations.
Do you have any references at hand?
But the partial updates is where I'd be concerned.
Yes, indeed.
Also worth noting here: in this blog post by @wycats there are shown benchmark results of Glimmer vs. Preact/React/Inferno based on this very benchmark, which suggest that Glimmer is substantially faster for "partial update": https://yehudakatz.com/2017/04/05/the-glimmer-vm-boots-fast-and-stays-fast/
But the published results as well as all the screenshots here do not confirm this!? 🤔
But I'm wondering, why glimmer-vm beat this gap using identity?
Indeed. Especially since in the previous versions Ember and Glimmer were pretty close together (though relatively slow), see my screenshot.
@krausest 555 pr says - it's keyed (for glimmer), for current version of this pr is keyed too.
Regarding versions - I'm trying to follow this https://github.com/krausest/js-framework-benchmark/pull/482 readme, I suggest it can be old results? Don't think preact
, react
results tested so mutch (only once - yarn && yarn build-prod
+ yarn selenium --count 3 --framework preact / react
.
in prev version with key="@index"
- not keyed
@NullVoxPopuli yeah, it make sence, also I found rendering bug in glimmer implementation with @identity
key -> after first removal view won't update for other removals. Replacing key from @identity
to id
helped to solve this problem.
Using mutation observer:
key=@identity
key=@index
@pzuraq
Thanks. Results follow the picture from the glimmer PR and don't look as good as in the first screenshots in that PR.
@krausest thank you! Yeah, first screenshot taken using key="@index"
wich is not relevant for keyed case (as we found out).
prev results: https://github.com/krausest/js-framework-benchmark/pull/482