Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(trace-of-tab): remove DevTools stableSort dependency #5532

Merged
merged 3 commits into from
Jun 25, 2018
Merged

core(trace-of-tab): remove DevTools stableSort dependency #5532

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe85acb
core(trace-of-tab): remove DevTools stableSort dependency
patrickhulce Jun 20, 2018
67bf316
fixup
patrickhulce Jun 21, 2018
5170316
pack them arrays
patrickhulce Jun 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 38 additions & 16 deletions lighthouse-core/gather/computed/trace-of-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,35 @@ class TraceOfTab extends ComputedArtifact {
return 'TraceOfTab';
}

/**
* @param {LH.TraceEvent[]} traceEvents
* @param {(e: LH.TraceEvent) => boolean} filter
*/
static filteredStableSort(traceEvents, filter) {
// 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])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.``

Copy link
Collaborator Author

@patrickhulce patrickhulce Jun 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW https://jsperf.com/inline-filter-vs-filter, but filter component is like 1/20th the cost of the sort

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

bench-plot

All of this is dependent on inlining decisions, though, and how lighthouse itself is run, so perf seems fine either way)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hot chart!

(yeah the code style doesn't bother me too much)

indices.push(srcIndex);
}
}

// sort by ts, if there's no ts difference sort by index
indices.sort((indexA, indexB) => {
const result = traceEvents[indexA].ts - traceEvents[indexB].ts;
return result ? result : indexA - indexB;
});

// create a new array using the target indices from previous sort step
const sortedArray = new Array(indices.length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use a const sortedArray = []; and push the new elements to get a packed array at the end

Copy link
Collaborator Author

@patrickhulce patrickhulce Jun 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two inconsistent comments are we making micro optimizations or not ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested and perf looks pretty much the same either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (let i = 0; i < indices.length; i++) {
sortedArray[i] = traceEvents[indices[i]];
}

return sortedArray;
}


/**
* Finds key trace events, identifies main process/thread, and returns timings of trace events
* in milliseconds since navigation start in addition to the standard microsecond monotonic timestamps.
Expand All @@ -41,16 +70,12 @@ class TraceOfTab extends ComputedArtifact {
async compute_(trace) {
// Parse the trace for our key events and sort them by timestamp. Note: sort
// *must* be stable to keep events correctly nested.
/** @type Array<LH.TraceEvent> */
const keyEvents = trace.traceEvents
.filter(e => {
return e.cat.includes('blink.user_timing') ||
e.cat.includes('loading') ||
e.cat.includes('devtools.timeline') ||
e.cat === '__metadata';
})
// @ts-ignore - stableSort added to Array by WebInspector.
.stableSort((event0, event1) => event0.ts - event1.ts);
const keyEvents = TraceOfTab.filteredStableSort(trace.traceEvents, e => {
return e.cat.includes('blink.user_timing') ||
e.cat.includes('loading') ||
e.cat.includes('devtools.timeline') ||
e.cat === '__metadata';
});

// Find the inspected frame
const {startedInPageEvt, frameId} = TracingProcessor.findTracingStartedEvt(keyEvents);
Expand Down Expand Up @@ -102,14 +127,11 @@ class TraceOfTab extends ComputedArtifact {

// subset all trace events to just our tab's process (incl threads other than main)
// stable-sort events to keep them correctly nested.
/** @type Array<LH.TraceEvent> */
const processEvents = trace.traceEvents
.filter(e => e.pid === /** @type {LH.TraceEvent} */ (startedInPageEvt).pid)
// @ts-ignore - stableSort added to Array by WebInspector.
.stableSort((event0, event1) => event0.ts - event1.ts);
const processEvents = TraceOfTab
.filteredStableSort(trace.traceEvents, e => e.pid === startedInPageEvt.pid);

const mainThreadEvents = processEvents
.filter(e => e.tid === /** @type {LH.TraceEvent} */ (startedInPageEvt).tid);
.filter(e => e.tid === startedInPageEvt.tid);

const traceEnd = trace.traceEvents.reduce((max, evt) => {
return max.ts > evt.ts ? max : evt;
Expand Down