[lpardosixtosMs]

This PR prepares the standalone TodoMVC implementations to be embedded in the Complex DOM UI.

• Adds a targeted classname and a li-<index> classname to all the TodoMVC list items. Required for triggering the generated CSS rules.
• Adds a targeted classname and a view-<index> classname to the direct children of all the TodoMVC list items. Required for triggering the generated CSS rules.
• Modifies the structure of the html tags so the final DOM is the same in all versions (except angular).

[lpardosixtosMs]

@flashdesignory In all but one implementation (backbone). The li-<index> of all the list elements is updated when deleting entries, this is a side-effect of using their position in the list to enumerate. IIUC this can cause extra style invalidation when deleting objects, I don't know if it is relevant for the benchmark, but I'd like to know your opinion.

[rniwa]

As sanity checks, can we get before/after numbers on each browser?

[bgrins]

Just quickly checking locally I'm seeing some pretty large slowdowns on a number of tests. I would have expected only a bit slower with the extra bookkeeping. I wonder if this has to do with #239 (comment), and if so, if there may be a more efficient way to get the effect we want in terms of providing matching selectors for the complex CSS. Will take a closer look at it tomorrow.

[bgrins]

I suspect what's happening is that the deleting step gets a lot slower since we loop through from 0 to 100. So when the first item is removed there's some new framework-specific re-render work necessary for the next 99 items and so on.

Thinking about how a todo app would likely work in the wild, I think this ID would be stable regardless of its position in the list (as it would i.e. be fetched from a database). This may be inconvenient to change but I have a couple ideas:

1. Come up with some other ID scheme that still allows us to target the items with CSS but is stable regardless of the position in the list. My first thought is some kind of hash based on the todo text. Since this is known ahead of time we could change the CSS codegen to use the same function - roughly hash(`Something to do ${i}`) instead of i.
2. Change the delete steps to do for (let i = numberOfItemsToAdd - 1; i >= 0; i--) to avoid this getting measured.

[bgrins]

I've prototyped approach 2 (which I think would be the easiest change to make) in https://reverse-removal--speedometer-preview.netlify.app. This can be compared with the current changes in the PR at https://lpardosixtos-addtodomvccomplexdomchanges--speedometer-preview.netlify.app/

[bgrins]

Results from a MacBook Pro using crossbench as follows. The total scores are roughly the same with the reverse removal applied, although in practice what's happened is most tests have gotten a bit slower (as I would have expected) and the existing React Complex DOM test has gotten a lot faster. This is because that test is already shipping the behavior with the re-renders during removal and so this optimizes that back to baseline / matching the current behavior on tip for the other Todo workloads)

Current score: poetry run cb sp3 --local "https://speedometer-preview.netlify.app/" --browser=firefox --browser=chrome --browser=safari --repeat=3 --stories=TodoMVC-Angular,TodoMVC-Backbone,TodoMVC-JavaScript-ES5,TodoMVC-JavaScript-ES6-Webpack,TodoMVC-JavaScript-ES6,TodoMVC-Preact,TodoMVC-React-Complex-DOM,TodoMVC-React-Redux,TodoMVC-React,TodoMVC-Svelte,TodoMVC-Vue,TodoMVC-jQuery

With the reverse-removal commit on top of the current PR: poetry run cb sp3 --local "https://reverse-removal--speedometer-preview.netlify.app/" --browser=firefox --browser=chrome --browser=safari --repeat=3 --stories=TodoMVC-Angular,TodoMVC-Backbone,TodoMVC-JavaScript-ES5,TodoMVC-JavaScript-ES6-Webpack,TodoMVC-JavaScript-ES6,TodoMVC-Preact,TodoMVC-React-Complex-DOM,TodoMVC-React-Redux,TodoMVC-React,TodoMVC-Svelte,TodoMVC-Vue,TodoMVC-jQuery

With the current PR: poetry run cb sp3 --local "https://lpardosixtos-addtodomvccomplexdomchanges--speedometer-preview.netlify.app/" --browser=firefox --browser=chrome --browser=safari --repeat=3 --stories=TodoMVC-Angular,TodoMVC-Backbone,TodoMVC-JavaScript-ES5,TodoMVC-JavaScript-ES6-Webpack,TodoMVC-JavaScript-ES6,TodoMVC-Preact,TodoMVC-React-Complex-DOM,TodoMVC-React-Redux,TodoMVC-React,TodoMVC-Svelte,TodoMVC-Vue,TodoMVC-jQuery

[bgrins]

Note that the reverse removal commit also appears to fix an existing bug in the Svelte test where all items are not being deleted (Step through on https://speedometer-preview.netlify.app/interactiverunner?suite=TodoMVC-Svelte vs https://reverse-removal--speedometer-preview.netlify.app/interactiverunner?suite=TodoMVC-Svelte), so there is marginally more work happening in that patch though the time stays pretty much the same across browsers.

[bgrins]

@rniwa @flashdesignory thoughts? Should we move forward after applying bgrins@f6d5f91 on top this?

[flashdesignory]

@rniwa @flashdesignory thoughts? Should we move forward after applying bgrins@f6d5f91 on top this?

Thanks for the quick solution. This makes sense to me and I'm voting for applying the patch.

[rniwa]

Can we get new before/after numbers?

[bgrins]

Can we get new before/after numbers?

the first two images in #239 (comment) have before/after if we take the suggested fix in #239 (comment)

[rniwa]

Alright, I've collected data of my own on 16" M2 MacBookPro. It looks like this change is neutral overall.

• Before: https://speedometer-preview.netlify.app/?tags=todomvc#details
• After: https://reverse-removal--speedometer-preview.netlify.app/?tags=todomvc#home

Chrome

• Before

  • 38.5 ± 2.4
  • 39.3 ± 2.6
  • 43.7 ± 3.2
  • 43.5 ± 3.2
  • 40.9 ± 1.1

• After

  • 43.6 ± 3.8
  • 40.9 ± 3.0
  • 40.0 ± 0.88
  • 42.7 ± 1.4
  • 40.2 ± 2.5

Firefox

• Before

  • 33.0 ± 1.6
  • 32.8 ± 1.6
  • 31.9 ± 1.4
  • 32.5 ± 1.5
  • 34.8 ± 2.1

• After

  • 31.4 ± 2.2
  • 33.3 ± 2.4
  • 31.9 ± 1.6
  • 32.6 ± 1.5
  • 32.3 ± 1.3

Safari

• Before

  • 33.9 ± 1.6
  • 34.5 ± 1.4
  • 34.4 ± 0.96
  • 34.9 ± 0.84
  • 34.6 ± 1.4

• After

  • 36.3 ± 2.0
  • 33.8 ± 0.98
  • 33.4 ± 1.3
  • 36.9 ± 1.1
  • 34.0 ± 1.2

reverse-removal.zip

[flashdesignory]

One question I had regarding the changes:
Adding unique classes to the todo items feels more like we should be using ids instead.
Classes are usually used to style elements that are not unique and ids are usually used to style elements that are.

Current proposed changes are something like below:

<li class="targeted li-${index}">
    <div class="targeted view-${index}">
        [...]
    </div>
</li>

Could we do something like this instead?

<li id="todo-li-${index}" class="targeted">
    <div id="todo-view-${index}" class="targeted">
        [...]
    </div>
</li>

[flashdesignory]

actually, disregard my comment, I think I see the reasoning why...

[bgrins]

@lpardosixtosMs alright, sounds like we can land this once you include the reverse removal change (bgrins@f6d5f91)

[lpardosixtosMs]

@bgrins I included the reverse removal change, it is ready to land. Thanks for the review!