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: faster saveTrace by streaming 500 events at a time #5387

Merged
merged 4 commits into from
Jun 11, 2018
Merged

core: faster saveTrace by streaming 500 events at a time #5387

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41d11d5
core: faster saveTrace by streaming 500 events at a time
paulirish May 30, 2018
aa44fd7
COMPAT comment
paulirish May 30, 2018
03896a5
SO link.
paulirish May 30, 2018
24938b3
--noneOfThis
paulirish May 31, 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
18 changes: 15 additions & 3 deletions lighthouse-core/lib/asset-saver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,12 +194,14 @@ async function prepareAssets(artifacts, audits) {
}

/**
* Generates a JSON representation of traceData line-by-line to avoid OOM due to
* very large traces.
* Generates a JSON representation of traceData line-by-line to avoid OOM due to very large traces.
* COMPAT: As of Node 9, JSON.parse/stringify can handle 256MB+ strings. Once we drop support for
* Node 8, we can 'revert' PR #2593. See https://stackoverflow.com/a/47781288/89484
Copy link
Member

Choose a reason for hiding this comment

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

Once we drop support for Node 8, we can 'revert' PR #2593

not sure if we actually want to anymore...we like the event-per-line formatting, right?

(We could consider shipping the pretty-json-stringify we just used in the snyk snapshot thing—no dependencies and 2.43KB before minification!—but it looks like using it on large traces will depend on Node 9 as well)

* @param {LH.Trace} traceData
* @return {IterableIterator<string>}
*/
function* traceJsonGenerator(traceData) {
const EVENTS_PER_ITERATION = 500;
const keys = Object.keys(traceData);

yield '{\n';
Expand All @@ -211,9 +213,19 @@ function* traceJsonGenerator(traceData) {
// Emit first item manually to avoid a trailing comma.
const firstEvent = eventsIterator.next().value;
yield ` ${JSON.stringify(firstEvent)}`;

let eventsRemaining = EVENTS_PER_ITERATION;
let eventsJSON = '';
for (const event of eventsIterator) {
yield `,\n ${JSON.stringify(event)}`;
eventsJSON += `,\n ${JSON.stringify(event)}`;
eventsRemaining--;
if (eventsRemaining === 0) {
yield eventsJSON;
eventsRemaining = EVENTS_PER_ITERATION;
eventsJSON = '';
}
}
yield eventsJSON;
}
yield '\n]';

Expand Down