[perf] batch-destroy

[lifeart]

Main idea here is to remove unnecessary work if we need to sync rendered list with new list with 0 length (list removal case, may be found in real-life applications where we switching between categories, or filtering / searching items).

Proposed changes reduce function nesting for this case, and at least remove 2 code branches per opcode (list item).

Possible improvement: (require vm updates)

if we had "list-start", "list-end" DOM marks, we could improve clear function, and instead of iterating over per-opcode bounds, we could just remove all from "list-start" node to "list-end" node
https://github.com/glimmerjs/glimmer-vm/blob/main/packages/%40glimmer/runtime/lib/bounds.ts#L54

UPD: looks like we could clear bounds itself.

Gives us in total 3 branches improvements per opcode.

---

Guessing here - since we don't remove expensive operations, such as destructors and one-to-one node list removal (instead of parent node), I would expect up to 5% in remove all case boost.

[NullVoxPopuli]

Some concerns brought up from @runspired :

• we may want to feature flag this, not because it's "breaking", but because folks may have written code that assumes accessing the parent node of the lint.
  • for example, element.parentElement.removeEventListener -- I (NVP) don't think folks should have been doing this in the first place, because if you're having your list-items mess with elements' parents ... that's just a perf waste -- just add the listener logic to the parent element.
  • also, document.body.querySelector -- again I (NVP) don't think folks should be doing this, but it is something that could break -- so it may be important we communicate that somehow.
  • @runspired's examples were tooltips, popovers, resize, scroll and viewport listeners
    These may be the bulk of what we'd have to look out for as complex library code that isn't managed by the average dev can do some wonky stuff, and be out of reach for modification / inspection by the average dev.

Perf

Notes:

• glimmer-vm:
  • baseline: a0c6c01 - v0.86.0
  • experiment: 8c96913 - clear bounds? (two commits ahead of baseline)
• ember-source:
  • baseline: Bump all glimmer-vm dependencies to 0.87.1 emberjs/ember.js#20609
  • experiment: Bump all glimmer-vm dependencies to 0.87.1 emberjs/ember.js#20609 with glimmer swapped out for 8c96913 (the glimmer-vm experiment commit)

[lifeart]

@NullVoxPopuli in my understanding we don't remove parentElement here, and in addition, we do removal after all destructors for list items is executed, concern is valid, but it seems not related to code changes.

There is only one possible case I was thinking - where opcodes in list has different parent or we have "some node" in the middle of the list.

Something like:

{{#each @items as |item|}}
  {{yield item}}
  {{#in-element item.node}} foo-bar {{/in-element}}
{{/each}}

But, it seems yield has it's own destructor opcode, and in-element too.

[lifeart]

Comments from discord (related to perf comparison):

clear code branches is not included to creation (should not side on this stuff), I would propose to change ordering of tests after some cooling time, technically first results may be faster because of CPU 🙂

if we see 14% improvement in only one (and expected use-case) and 5-17% regression for all cases, it may be a case where we actually more than 14% faster in "all nodes removal", but because of CPU throttling (let it be 10%), we see degradation in other metrics

[lifeart]

it seems replace elements this.args = [] not working this way, it's works just like append.

[1,3,4]   ->  [5,6] --=> [5,6,1,3,4]   -> [5,6]
old list  ->  prepend new items        -> remove old items one-by-one

[lifeart]

Closing because it require VM list change