split into 2 tables "keyed" and "non-keyed" ? #104
Comments
|
@leeoniya I expected the non-keyed impl of vanilla-js to perform faster for remove than the keyed impl, but the reported duration didn't show that. It turned out that if I revert the icon resizing transition the results are as expected a bit faster for non-keyed. I haven't reverted the css in the repo yet. There are a few implementations that behave like keyed for the remove operation that run very fast for replace all rows. I'll have to look into that the next few days... |
|
@leeoniya It behaves like non-keyed for repeated create 1000 row (the red row stays visible). |
|
thanks! i'll take a look at Ractive tonight. i can help you debug to ensure all the impls are doing the proper dom ops. there are several keyed impls < 140.83 for "replace all rows"...which is highly suspect. the keyed vs non-keyed explanation looks good. BTW, is there somewhere where all the impls are compiled? It's a huge PITA building everything from scratch. |
|
@krausest |
|
ok, I tested and as expected these are non-keyed (do not perform 1000 x removeChild or replaceChild calls):
i could not get the other two to either build or run, but there's zero chance of them actually being keyed given their low timings:
the fact that you can trick ractive into a "keyed" mode is interesting but i could not find any official keyed mode in the ractive docs and if it takes some kind of hack to get it to work this way it's probably not well suited for the task. ractive does behave as keyed for single removal, but not for mass clearing. i assume that the difference has to do with the actual click event serving as a hint for which dom element needs to be removed, whereas there's no such hint when you clear/replace the full dataset programmatically. @trotyl lib/implementation authors are free (and encouraged) to submit both keyed and non-keyed versions if both are supported. |
|
@leeoniya I have a question that is index-keyed being regarded as keyed or non-keyed? In some frameworks, an explicit statement is required for tracking them by index, letting it looks like keyed in syntax, but the key is not provided by data at all. And in some other frameworks, it implicitly uses the same mechanism for tracking data by index and do not require extra statement. Should there be some clarification about the terminologies like keyed or non-keyed so that others could contribute more correctly? |
|
@trotyl the distinction is simple: keyed implementations must not recycle/reuse dom nodes for differing keys (or object identities). how this requirement is satisfied by each framework and implementation is not prescribed. but generally this means that all keyed implementations will have to physically remove and insert new dom nodes for "replace all rows". non-keyed implementations may simply update the existing dom nodes in-place to match the new data. |
|
@leeoniya @trotyl If the first row has some color after the run operation, the benchmark belongs to the non-keyed group. Add code that changes the row's color that should deleted in "remove row": If the row where delete is click is colored, the benchmark belongs to the non-keyed group. This also works nicely for react when using index as the key. This react impl would belong to the non-keyed group, which is right in my opinion. I checked the impact of the two operations. The "-2" version has the code added, the other not. Seems like it has no impact on the replace all or remove row benchmark and all frameworks would have the same disadvantage. |
|
👎 from me. I don't think it's necessary to introduce convoluted hacks try to force frameworks to behave a specific way. If there is no keyed impl available or possible, then that's just a fact of life. This one's more controversial, but I also don't think it's necessary to provide a visual way for authors to test whether their impl is keyed or non-keyed. The guideline is pretty simple (don't reuse/recycle dom nodes across different keys or object idents) and if they don't know which impl they are submitting (hopefully they do!), then we can classify it for them using some very basic DOM instrumentation. This means you can also remove the css transition on the You can classify any framework automatically at runtime. Simply instrument if using DOMInstr [1]... // render 10 rows...
var instr = new DOMInstr(true);
instr.start();
// run "replace all rows"
// account for async impls
setTimeout(function() {
var counts = instr.end();
if (counts.insertBefore + counts.appendChild + counts.replaceChild < 10) {
// non-keyed
}
else {
// keyed
}
}, 200);[1] https://github.com/leeoniya/domvm/blob/2.x-dev/test/dominstr.js |
|
@leeoniya I'll look into automatic classification from the webdriver test driver. Do you think DOMInstr detects changes for shadow dom (polymer caused a lot of pain for webdriver so I'm alarmed). What advantage would DOMInstr have against MutationObserver? |
it requires modifying the current impls and complicating future impls for the sake of classification, which can easily be done without introducing anything special.
most likely nothing, mutationobserver is probably better if available. dominstr was made to be compatible with unit testing for ie9+ |
|
Understood - and yes of course. I'll look into it. The advantage of coloring the dom is that it illustrates the effect nicely also for people who aren't in the depths of keyed vs. non-keyed. I wouldn't have considered that sufficiently when choosing a framework and it might turn out as a big pain when working with external libs or introducing transitions. |
|
i agree that keyed vs non-keyed is not readily obvious and there should be a separate demo repo which visually illustrates the difference and the bugs that non-keyed can introduce. i dont think it's necessary for every implementation to reproduce a subset of possible bugs. |
|
@krausest i have verified that
|
|
I've added webdriver-ts/nonKeyed.ts (invoked with npm run compile && node dist/nonKeyed.js in the webdriver-ts folder). It uses a MutationObserver and checks the kind of modification replace all rows causes and whether remove row removes the specific row that should be removed. The transition on the delete icon has been removed as it had a negative impact on the remove row benchmark. It prints the following output: which is consistent with the findings so far. The results table has been updated. |
|
👍 looks like we now have ourselves a useful benchmark, sir :) |
|
@krausest as a separate but related note, it may be worth deciding on a structural nomenclature for i propose appending ...or you can just leave it the way it is. thoughts? |
|
I'd introduce a naming convention only for frameworks that have both a keyed an a non-keyed submission. In that case I prefer the suffixes "-keyed" and "-non-keyed". |
|
sounds good. the results table may look cluttered if every header has suffixes. since they're now split up, the distinction doesnt need to be explicitly stated in each case. i guess you could strip the predefined suffixes during the table generation? |
|
Absolutely. |
|
I implemented the suffix removal, but somehow I don't like it when I looked at the results. It's clearer what implementation the results are for when the suffix is kept and it doesn't look too bad in the table (see angular 2). |
|
what about using the empty upper left cell in the header to say keyed or non-keyed and make it highly visible like blue background/white text. and different distinctive colors for non-keyed.
|
Looking at the table [which is getting way too wide IMO], the perf numbers are very difficult to compare because many impls are keyed and many are unkeyed. these are really 2 completely different perf profiles.
if you prefer to have a single huge table at the very least, it would make sense to order/group them by keyed vs non-keyed. a trivial way to see this is to assume everything < 130ms for "replace all rows" is unkeyed and > 130ms is keyed.
i think 2 tables will make sense and would provide much more meaningful comparisons (and also not span a 2 x 1080p screens :)
the vanillajs-keyed impl would need to be fixed for this as well: see #103
The text was updated successfully, but these errors were encountered: