core(unused-javascript): augment with source maps #10090

connorjclark · 2019-12-11T00:29:47Z

http://misc-hoten.surge.sh/lh-reports/pr-10090.html

node lighthouse-cli https://www.coursehero.com/sg --config-path lighthouse-core/config/experimental-config.js --only-audits unused-javascript

connorjclark · 2019-12-11T00:30:39Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+    /** @type {Record<string, number>} */
+    const files = {};
+
+    // This is 10x slower (~320ms vs 46ms for a big map), but its correctness


Left this in for reviewers. will remove later.

paulirish · 2019-12-11T22:32:00Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+      let offset = lineOffsets[mapping.lineNumber];
+
+      offset += mapping.columnNumber;
+      const byteEnd = mapping.lastColumnNumber || lineLengths[mapping.lineNumber];


byteEnd is a lastColumnOfLine ?

lastColumnOfMapping

paulirish · 2019-12-11T22:32:37Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+      let offset = lineOffsets[mapping.lineNumber];
+
+      offset += mapping.columnNumber;
+      const byteEnd = mapping.lastColumnNumber || lineLengths[mapping.lineNumber];


lengths are 1 indexed but columnNums are 0 index so i think you want to decrement one if you're pulling from lineLengths?

fixed. The debugging code I added made it easy to confirm the bytes are counted correctly.

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+    for (const mapping of bundle.map.mappings()) {
+      let offset = lineOffsets[mapping.lineNumber];
+
+      offset += mapping.columnNumber;


paulirish · 2019-12-11T22:36:40Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+      offset += mapping.columnNumber;
+      const byteEnd = mapping.lastColumnNumber || lineLengths[mapping.lineNumber];
+      for (let i = mapping.columnNumber; i < byteEnd; i++) {
+        if (wasteData.every(data => data.unusedByIndex[offset] === 1)) {


i guess the alternative to this is if the mapping could also express its start and endOffsets?

i'm interested in what'd be involved in making that happen.

It would be straight-forward, but would need to be done upstream in CDT. The offset can be pulled from the character iterator's position (see SourceMap._parseMap).

This current method is good for now. when you bring this feature to CDT it'll be good if you can make the unused calculation re-usable, then we can delete this function.

This current method is good for now. when you bring this feature to CDT it'll be good if you can make the unused calculation re-usable, then we can delete this function.

lol so you're saying that what you did is fine, but i should reimplement when i upstream it? 🤣

i guess you're taking this position mostly because the changes to sdk/sourcemap.js would be too significant? i guess that makes sense, though the class now is pretty modified already. ah well.

There's two parts here:

modify SourceMap._parseMap to also save the end offsets, so the byte attribution can be a little faster

the feature you're working on in CDT will need to do the same byte attribution. So there is no difference in LH vs CDT, this should be done in the same way by using the same code

I don't want to do 1. here because the change needed is much more than how this PR is currently augmenting SourceMap/SDK. Would need to completely replace a private function. Since it's just for speed, not correctness, I think it can wait until done upstream.

For 2, I'd imagine you'll just copy L90-116 here, and make a function on SourceMap, something like calculateSourceByteAttribution(compiledSource: string). I'm lost on if you've done this bit already in your CL - are you already calculating attribution for bundle?

also, I don't mind taking over the above within DT :)

also, I don't mind taking over the above within DT :)

that sounds pretty divine to me.

lighthouse-core/audits/byte-efficiency/unused-javascript.js

commit ecb6260 Author: Connor Clark <cjamcl@google.com> Date: Wed Dec 11 15:17:52 2019 -0800 no index commit 52796db Author: Connor Clark <cjamcl@google.com> Date: Wed Dec 11 15:17:02 2019 -0800 no unshift commit 943e19c Author: Connor Clark <cjamcl@google.com> Date: Wed Dec 11 15:14:21 2019 -0800 deletionBraceCount commit caa269b Author: Connor Clark <cjamcl@google.com> Date: Wed Dec 11 15:13:25 2019 -0800 comments commit e6fde71 Author: Connor Clark <cjamcl@google.com> Date: Wed Dec 11 15:02:50 2019 -0800 semver commit 6480198 Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 21:18:36 2019 -0800 update frontend commit 5d17268 Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 21:07:44 2019 -0800 rm some ts ignores commit 7129ef9 Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 21:04:09 2019 -0800 trim unneeded code commit 0b655b6 Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 20:46:11 2019 -0800 better cdt lib gen commit 3333c4f Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 20:19:20 2019 -0800 add sections to source map type commit 73eca7b Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 20:00:02 2019 -0800 move SDK up b/c global pollution is gone commit 47e8a41 Author: Connor Clark <cjamcl@google.com> Date: Tue Dec 10 19:44:59 2019 -0800 comment commit 9f47880 Author: Connor Clark <cjamcl@google.com> Date: Mon Dec 9 15:02:57 2019 -0800 remove unused js commit 7d50227 Author: Connor Clark <cjamcl@google.com> Date: Sat Dec 7 00:36:07 2019 -0800 nul commit ed4bb21 Author: Connor Clark <cjamcl@google.com> Date: Sat Dec 7 00:33:39 2019 -0800 remove ParsedURL commit 43da869 Author: Connor Clark <cjamcl@google.com> Date: Sat Dec 7 00:30:08 2019 -0800 tmp commit 0d73a8b Author: Connor Clark <cjamcl@google.com> Date: Sat Dec 7 00:22:10 2019 -0800 reduce code commit 4a9127d Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 23:52:44 2019 -0800 delete some stuff not needed commit dff8358 Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 23:24:48 2019 -0800 errors commit 808736d Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 22:56:20 2019 -0800 Common.console commit 122f2a2 Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 22:54:42 2019 -0800 ts commit 2f80d5a Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 22:50:18 2019 -0800 remove hacky globalThis.cdt commit 7ad9389 Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 21:24:00 2019 -0800 do not pollute global array commit fa07a80 Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 21:17:56 2019 -0800 mapping test commit f9408f3 Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 20:49:23 2019 -0800 count lines commit 6d27caa Author: Connor Clark <cjamcl@google.com> Date: Fri Dec 6 20:41:09 2019 -0800 initial

connorjclark · 2020-02-20T19:49:18Z

Updated to use optional artifacts. Will be no change when using the default config. Will see bundle stuff when using the experimental config.

Gating moving to default until better styling for subRows. And I guess at this point, it might be best to just release (read: move to the default config) all the source map stuff at once (including visualization).

First post updated with link to a report.

paulirish · 2020-02-22T00:08:20Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+      offset += mapping.columnNumber;
+      const byteEnd = mapping.lastColumnNumber || lineLengths[mapping.lineNumber];
+      for (let i = mapping.columnNumber; i < byteEnd; i++) {
+        if (wasteData.every(data => data.unusedByIndex[offset] === 1)) {


also, I don't mind taking over the above within DT :)

that sounds pretty divine to me.

paulirish · 2020-02-22T00:10:10Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+   * @param {ReturnType<typeof UnusedJavaScript.determineLengths>} lengths
+   * @param {number} bundleSourceUnusedThreshold
+   */
+  static createBundleMultiData(item, wasteData, bundle, lengths, bundleSourceUnusedThreshold) {


iirc you had more descriptive function names before.

i think this should be two functions..
one is L92 - 118... and titled around computing bundle waste ish
and then a second is L120 - 138 and calling that bundle subrow data ish sgtm

you said you're about to extract some of this into a computed artifact, so it sounds like you're going to do this in your next PR anyhow, yeah?

yeah, I have a few better names and split this fn in a computed artifact PR :)

lighthouse-core/audits/byte-efficiency/unused-javascript.js

paulirish · 2020-02-22T00:16:06Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+  /**
+   * @param {WasteData[]} wasteData
+   * @param {string} url
+   * @param {ReturnType<typeof UnusedJavaScript.determineLengths>} lengths


wow that's wild.

paulirish · 2020-02-22T00:21:30Z

lighthouse-core/test/config/config-test.js

-      assert.deepStrictEqual(config, extendedConfig, 'had mutations');
+      // Clone so we do not compare event callbacks binded on gatherers.
+      assert.deepStrictEqual(
+        JSON.parse(JSON.stringify(config)),


this wipes out a bunch of checks on these objects, so i'm a bit hesitant to loosen things this much..

whats the motivation?

the source map gatherer sets a .bind property in its ctor (so two instances are always gonna be not equal). i think this test broke when I had source maps in the default config ... but the plan has changed since so i'll revert this change. Will prop up again when we move maps to default config..

We ran into this again in #10623

The should generate the same filtered config, extended or original test in config-test.js will break if there's any .bind() property in the ctor (like @connorjclark said). This test filters down to onlyCategories: ['performance'],.. and the performance category, previous to 10090 and 10623, had ZERO gatherers with bound instance props. (Just by coincidence). Whereas best-practices and other categories definitely have them.

So this was a fun lil unexpected failure.

In #10623, @Beytoven is adding some code to delete instances before doing the diff which will sort this out.

paulirish · 2020-02-22T01:31:23Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

-        {key: 'totalBytes', valueType: 'bytes', label: str_(i18n.UIStrings.columnSize)},
-        {key: 'wastedBytes', valueType: 'bytes', label: str_(i18n.UIStrings.columnWastedBytes)},
+        /* eslint-disable max-len */
+        {key: 'url', valueType: 'url', subRows: {key: 'sources', valueType: 'code'}, label: str_(i18n.UIStrings.columnURL)},


if the valueType can be url then it'd fix our text wrapping situation here:

repro: node lighthouse-cli https://elsoberano.org/ --config-path lighthouse-core/config/experimental-config.js --only-audits unused-javascript -GA

I think we can safely use url

thanks to this code:

i somewhat remember discussing how we handle sourcesRoot and doing a new URL(url, base) thing so that'd definitely play a role here for relative source paths.....

i think amp is very unusual in how they do their maps.

i think the goal here is to reduce the length of each sub row...for that, we can trim off the longest common prefix (LCP ;) as we collect the source map. all other uses of maps will benefit from this too.

paulirish

only last item. after this we gucci.

paulirish · 2020-02-25T01:49:36Z

lighthouse-core/gather/gatherers/source-maps.js

+  /**
+   * @param {LH.Artifacts.SourceMap} SourceMap
+   */
+  trimCommonPrefix(SourceMap) {


i was expecting to see the trimming done in the audit as it's a presentational concern.

could we move it there?
i'd feel better if our sourcemap artifacts remain as we received them.

let's do it in jsbundles. I think every audit will want this treatment.

nvm lets do it ur way

paulirish

rip goat

connorjclark · 2020-02-25T04:18:47Z

Done running Lighthouse!
+lhci upload --serverBaseUrl=https://lhci-canary.herokuapp.com/ --token=[secure] --github-token=[secure]
Error: Unexpected status code 500
  <!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Internal Server Error</pre>
</body>
</html>
    at ApiClient._convertFetchResponseToReturnValue (/home/travis/.nvm/versions/node/v10.19.0/lib/node_modules/@lhci/cli/node_modules/@lhci/utils/src/api-client.js:55:21)
    at process._tickCallback (internal/process/next_tick.js:68:7)error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The command "yarn dogfood-lhci" exited with 1.

@patrickhulce D:

patrickhulce · 2020-02-25T15:33:20Z

thanks @connorjclark should be fixed now, time to move off the free plan I think, we hit the quota about twice a week now 😬

connorjclark and others added 9 commits December 9, 2019 15:17

initial

4a62a54

do not need _ yet

7886f5e

test for unused bundle

65cc13c

lint

2ed3b7b

fix null mapping bug

652059e

fix counting

b48a400

fix smoke test

7bdf8b4

comment

b6a4a08

comment

b840b10

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners December 11, 2019 00:29

googlebot added the cla: yes label Dec 11, 2019

connorjclark commented Dec 11, 2019

View reviewed changes

lint

c3fd09e

vercel bot deployed to staging December 11, 2019 03:50 View deployment

better bundle size filter

36c7bee

vercel bot deployed to staging December 11, 2019 03:56 View deployment

paulirish requested changes Dec 11, 2019

View reviewed changes

vercel bot deployed to staging December 12, 2019 04:35 View deployment

todo

912f9f1

vercel bot deployed to staging December 12, 2019 21:36 View deployment

update bundle.js

ea03907

vercel bot deployed to staging December 12, 2019 22:43 View deployment

count new lines

a862683

vercel bot deployed to staging December 12, 2019 23:36 View deployment

dont double count

4ff3804

vercel bot deployed to staging December 13, 2019 21:36 View deployment

fix test

fa429e0

Merge remote-tracking branch 'origin/master' into unused-js-bundles

5cbd43c

vercel bot deployed to Preview February 20, 2020 19:51 View deployment

updateg

2680116

vercel bot deployed to Preview February 21, 2020 01:10 View deployment

bundlesize

df5aac6

vercel bot deployed to Preview February 21, 2020 21:43 View deployment

paulirish reviewed Feb 22, 2020

View reviewed changes

const, revert config test

b917684

vercel bot deployed to Preview February 22, 2020 00:30 View deployment

lint

8b356cf

vercel bot deployed to Preview February 22, 2020 00:44 View deployment

connorjclark requested a review from paulirish February 22, 2020 00:44

paulirish reviewed Feb 22, 2020

View reviewed changes

trim lcp

01fa7f8

vercel bot deployed to Preview February 22, 2020 03:35 View deployment

connorjclark requested a review from paulirish February 22, 2020 04:04

paulirish requested changes Feb 25, 2020

View reviewed changes

connorjclark requested a review from paulirish February 25, 2020 02:21

yay

44f7fd8

vercel bot deployed to Preview February 25, 2020 02:22 View deployment

paulirish approved these changes Feb 25, 2020

View reviewed changes

connorjclark added land-when-ci-is-green and removed waiting4reviewer labels Feb 25, 2020

devtools-bot merged commit 672bb60 into master Feb 25, 2020

devtools-bot deleted the unused-js-bundles branch February 25, 2020 19:21

devtools-bot removed the land-when-ci-is-green label Feb 25, 2020

connorjclark mentioned this pull request Feb 27, 2020

JS analysis blockers ☂️ #10369

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(unused-javascript): augment with source maps #10090

core(unused-javascript): augment with source maps #10090

connorjclark commented Dec 11, 2019 •

edited

Loading

connorjclark Dec 11, 2019

paulirish Dec 11, 2019

connorjclark Dec 13, 2019

paulirish Dec 11, 2019

connorjclark Dec 13, 2019

This comment was marked as resolved.

paulirish Dec 11, 2019

connorjclark Dec 13, 2019

paulirish Feb 1, 2020

connorjclark Feb 1, 2020

connorjclark Feb 21, 2020

paulirish Feb 22, 2020

connorjclark commented Feb 20, 2020 •

edited

Loading

paulirish Feb 22, 2020

paulirish Feb 22, 2020

connorjclark Feb 22, 2020

paulirish Feb 22, 2020

paulirish Feb 22, 2020

connorjclark Feb 22, 2020

paulirish Apr 24, 2020

paulirish Feb 22, 2020

paulirish Feb 22, 2020

connorjclark Feb 22, 2020

paulirish Feb 25, 2020

paulirish left a comment

paulirish Feb 25, 2020

connorjclark Feb 25, 2020

connorjclark Feb 25, 2020

connorjclark Feb 25, 2020

paulirish left a comment

connorjclark commented Feb 25, 2020

patrickhulce commented Feb 25, 2020

core(unused-javascript): augment with source maps #10090

core(unused-javascript): augment with source maps #10090

Conversation

connorjclark commented Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

connorjclark commented Feb 25, 2020

patrickhulce commented Feb 25, 2020

connorjclark commented Dec 11, 2019 •

edited

Loading

connorjclark commented Feb 20, 2020 •

edited

Loading