Listeners coalesce runtime perf more info on tnode #29864
Conversation
pkozlowski-opensource
force-pushed
the
listeners_coalesce_runtime_perf_more_info_on_tnode
branch
from
475b8ac
to
55cc1d4
Compare
| /** | ||
| * Marks an ending index in the TView.cleanup structure where entries for a given node are stored. | ||
| */ | ||
| cleanupEnd: number; |
There was a problem hiding this comment.
An alternative might be to store 1 property on TNode that encodes the cleanupStart index in the first x bits and the cleanupEnd index in the last x bits, then unwrap the 2 indices in the listener logic. This would save us 1 property per TNode. Sacrifices some readability, but halves the memory footprint.
There was a problem hiding this comment.
I was afraid you are going to say this :-) Still, I'm really not big fan of merging this PR at all.
If you think that it is beneficial to merge it I would like to do review of data structures while doing perf testing. The thing is that currently TNode is mostly using combination of start / end indexes (directiveStart / directiveEnd, propertyMetadataStartIndex / propertyMetadataEndIndex) and the only place where we've got bitwise encoding (providerIndexes) has a comment // TODO(misko): break this into actual vars..
What I'm trying to say is: if we care about memory usage here we should unify it for the entire TNode. And if we decide to do standardise on using bitwise encoding I would prefer to do so while working on benchpress scenarios to have solid numbers on progress.
How does it sound?
| for (let i = 0; i < tCleanup.length - 1; i += 2) { | ||
| if (tCleanup[i] === eventName && tCleanup[i + 1] === tNodeIdx) { | ||
| for (let i = tNode.cleanupStart; i < tNode.cleanupEnd - 1; i += 2) { | ||
| if (tCleanup[i] === eventName && tCleanup[i + 1] === tNode.index) { |
There was a problem hiding this comment.
Is this check still necessary, given that we're only going through the cleanup functions for 1 TNode?
tCleanup[i + 1] === tNode.index
There was a problem hiding this comment.
I believe so since at this position we can store either a TNode index or a "target getter" for cases like (window:click).
| tNode.cleanupStart = tCleanup.length; | ||
| } | ||
| tCleanup.push(eventName, idxOrTargetGetter, listenerFnIdx, useCaptureOrIndx); | ||
| tNode.cleanupEnd = tNode.cleanupEnd + 4; |
There was a problem hiding this comment.
Wouldn't tNode.cleanupEnd be -1 initially? So the first time this runs, tNode.cleanupStart will be the startingtCleanup.length and tNode.cleanUpEnd will always be 3.
| tNode.cleanupEnd = tNode.cleanupEnd + 4; | |
| tNode.cleanupEnd = tNode.cleanupEnd < 0 ? tNode.cleanupStart + 4 : tNode.cleanupEnd + 4; |
Seems like a test should have caught this?
There was a problem hiding this comment.
Good catch, thank you! Slightly changing your suggestion to compare with -1 to be consistent with the checks above.
pkozlowski-opensource
force-pushed
the
listeners_coalesce_runtime_perf_more_info_on_tnode
branch
from
55cc1d4
to
1677d10
Compare
|
I don't think we intend to merge this PR as-is so closing for now |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
** Only review the second commit ** (the first commit is part of #29859).
The second commit is a runtime perf-optimisation that limits the scope of search in
TNode.cleanupwhen looking up existing listeners to coalesce. While it certainly improves things from CPU cycles point of view, sadly it does so at the expense of memory usage (and one could argue that CPU as well if we take GC into account).At this point I feel like adding 2 numbers to each and every
TNodeis too much to pay for CPU cycles optimisations here, but I would like to confirm with solid numbers from perf profiling. As such my recommendation would be to:Still, opening this WIP PR so we can discuss.