core(trace-of-tab): remove DevTools stableSort dependency #5532
Conversation
patrickhulce
force-pushed
the
remove_stable_sort_dep
branch
from
3321dd2
to
67bf316
Compare
| // create an array of the indices that we want to keep | ||
| const indices = []; | ||
| for (let srcIndex = 0; srcIndex < traceEvents.length; srcIndex++) { | ||
| if (filter(traceEvents[srcIndex])) { |
There was a problem hiding this comment.
does combining the filter here gain much in performance? It doesn't feel great combined (they don't really have much to do with each other, we just happen to usually do them in pairs) so unless it buys a lot it doesn't feel worth it
There was a problem hiding this comment.
it was 3-pronged
TraceOfTab.stableSort(
traceEvents.filter()
)was gross
2) avoid another copy of the array since we're introducing a new one here and there were recent issues with large traces hurting people
3) filter inline here was ever so mildly faster on big traces
none of them are particularly strong reasons, so if it's rubbing the wrong way I can go back
There was a problem hiding this comment.
I'm seeing the perf benefit of the inline filter to be substantial. I think all numbers are under 50ms even for big traces, but it's worth like a 35% speedup.``
There was a problem hiding this comment.
FWIW https://jsperf.com/inline-filter-vs-filter, but filter component is like 1/20th the cost of the sort
There was a problem hiding this comment.
ha, if you're fine on a style level with combining the two steps, @paulirish, I can be fine with it too. Testing it now I see ~5% improvement over all of _compute() for a 31MB verge trace (~138ms -> ~131). I was just questioning this part from a code-semantics perspective.
There was a problem hiding this comment.
(it's likely not better than 5% because the filteredStableSort call goes polymorphic with the second call to it in _compute(), while AIUI as of Node 10 [].filter() specializes at the callsite, not within.
This may get worse as more calls are made, e.g. if someone is running the node module in a loop. You can kind of simulate this by disabling the computed artifact cache and letting trace-of-tab run as many times as requested in a -G run (110 times!). In that case the new method is a little slower (by ~5.3±4ms)
All of this is dependent on inlining decisions, though, and how lighthouse itself is run, so perf seems fine either way)
There was a problem hiding this comment.
hot chart!
(yeah the code style doesn't bother me too much)
| }); | ||
|
|
||
| // create a new array using the target indices from previous sort step | ||
| const sortedArray = new Array(indices.length); |
There was a problem hiding this comment.
should use a const sortedArray = []; and push the new elements to get a packed array at the end
There was a problem hiding this comment.
two inconsistent comments are we making micro optimizations or not ;)
There was a problem hiding this comment.
tested and perf looks pretty much the same either way.
There was a problem hiding this comment.
two inconsistent comments are we making micro optimizations or not ;)
ha, I thought about that as I was writing it. In theory it helps, but I only suggested it because it's still idiomatic :)
tested and perf looks pretty much the same either way.
It might even be a little slower in filteredStableSort itself (because it has to realloc as the array grows). In theory the help is elsewhere as array accesses only have to do bounds checks, not hole checks as well.
| }); | ||
|
|
||
| // create a new array using the target indices from previous sort step | ||
| const sortedArray = new Array(indices.length); |
There was a problem hiding this comment.
tested and perf looks pretty much the same either way.
| // create an array of the indices that we want to keep | ||
| const indices = []; | ||
| for (let srcIndex = 0; srcIndex < traceEvents.length; srcIndex++) { | ||
| if (filter(traceEvents[srcIndex])) { |
There was a problem hiding this comment.
I'm seeing the perf benefit of the inline filter to be substantial. I think all numbers are under 50ms even for big traces, but it's worth like a 35% speedup.``
Summary
Further reducing our dependency on DevTools frontend.
Few notes: there were already tests for stable sorting, I manually did some additional dSE on thornier traces and compared execution time, this one is ~1-5% faster on my hardware than DevTools version, so no real difference perf-wise.
(left the one for mainthread breakdown since, #5533 takes care of it)