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.

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

core: replace WebInspector traceparser with native JSON.parse #6099

Merged
merged 1 commit into from
Sep 24, 2018

Conversation

brendankenny
Copy link
Member

possibly more controversial than #6090 and #6091

The reason we introduced a streaming trace parser was because we would get RangeError: Invalid string length with traces that were larger than 256MB, the previous maximum string size in V8.

This changed in V8 6.2, which increased the max size to 1GB. This landed in Node 8.10.0 in March, which is after the original LTS release of 8.9. However:

It thus seems reasonable to expect at least Node 8.10 to run Lighthouse if we're supporting at least the LTS branch. I've updated the package.json engines entry to give a warning on install in npm (not sure what the yarn engines behavior is these days).

// Run when in extension context, but not in devtools.
if ('chrome' in window && chrome.runtime) {
// Run when in extension context, but not in devtools or unit tests.
if (typeof window !== 'undefined' && 'chrome' in window && chrome.runtime) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this PR, lighthouse-ext-background.js was pulling in assetSaver, which was pulling in WebInspector, which was setting global.window = global, so this file was fine to pull into node unit tests.

With that dependency path removed, this protects us from erroring on the window access (in the actual extension the behavior is unchanged since window is already defined :)

@brendankenny
Copy link
Member Author

removes another 6KB off our gzipped bundle (508.62KB -> 502.56KB) since we can remove some of the other common/ and sdk/ files in addition to the parser itself.

@brendankenny
Copy link
Member Author

brendankenny commented Sep 24, 2018

oh and

  • it's faster to just JSON.parse the string. Loading artifacts with that 150MB trace takes about 2760ms on master for me, 1618ms after this PR

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

i'm good with the compat implications. all non-node clients are good already. only affects Node <8.10 and only in -A. -G and --save-assets are still good as they use the streaming trace-saver.

@brendankenny brendankenny merged commit 14d6450 into master Sep 24, 2018
@brendankenny brendankenny deleted the traceparser branch September 24, 2018 19:16
@patrickhulce
Copy link
Collaborator

heh, I was going to suggest this too :D nice!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants