cloudfour / lighthouse-parade

A Node.js command line tool that crawls a domain and gathers lighthouse performance data for every page.
MIT License
362 stars 14 forks source link

Update lighthouse to 9.5.0 #92

Closed calebeby closed 2 years ago

calebeby commented 2 years ago

In the previously published lighthouse-parade version, v1.1.0, we were usingl ighthouse 6.4.0. There was a previous PR (merged) that updated to 7.5.0, but that version was never published to npm. This PR updates lighthouse to 9.5.0.

Since lighthouse changed the columns a bit I created a new google sheets template with the new columns: https://docs.google.com/spreadsheets/d/1z8jY1lF2ERbNFW4XHSxNjege8kj5JIUp1f-nMD151HU/edit#gid=1662529665

And I have done a results comparison on my machine (non-M1) from lighthouse 6.4.0 to lighthouse 9.5.0. Command (node 14):

lighthouse-parade https://cloudfour.com --max-crawl-depth 3 --exclude-path-glob "/thinks/*" --exclude-path-glob "/presents/*"

Results from lighthouse 6.4.0 Results from lighthouse 9.5.0

Largely the results are similar, similar enough to not make me worried about merging this.

It would be helpful for someone with a mac M1 to test this PR out, and re-run the same command and see if this issue still happens.

changeset-bot[bot] commented 2 years ago

🦋 Changeset detected

Latest commit: 9f3c6ce2a864bf23a2c22786d3d9c935463fb4dc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ----------------- | ----- | | lighthouse-parade | Major |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

spaceninja commented 2 years ago

I'll test this out shortly

spaceninja commented 2 years ago

Dumb question — how do I run lighthouse-parade locally? When I try to run the command you listed above, it just says "lighthouse-parade not found", but of course, if I run the npx command from the README, it'll run the published version.

spaceninja commented 2 years ago

To test locally, you'll need to install this branch, build it, and run npm link:

  1. git checkout lh-9-5-0
  2. npm run build
  3. npm link
  4. lighthouse-parade https://cloudfour.com --max-crawl-depth 3 --exclude-path-glob "/thinks/*" --exclude-path-glob "/presents/*"
spaceninja commented 2 years ago

Results 9.5.0 M1 Node 16

calebeby commented 2 years ago

Great! It looks like that data matches the non-M1 data pretty well! So maybe this fixes #82. But also @spaceninja was not able to reproduce #82 so it probably makes sense for @Paul-Hebert (who was able to reproduce #82) to check out this branch to see if it fixes it on his machine

Paul-Hebert commented 2 years ago

Great! It looks like that data matches the non-M1 data pretty well! So maybe this fixes https://github.com/cloudfour/lighthouse-parade/issues/82. But also @spaceninja was not able to reproduce https://github.com/cloudfour/lighthouse-parade/issues/82 so it probably makes sense for @Paul-Hebert (who was able to reproduce https://github.com/cloudfour/lighthouse-parade/issues/82) to check out this branch to see if it fixes it on his machine

Set a reminder to do so tomorrow (though hopefully I'll have time later today)

calebeby commented 2 years ago

@spaceninja were you using node 14?

spaceninja commented 2 years ago

Nope, I was running v16.14.0

Need me to re-run in 14?

calebeby commented 2 years ago

Maybe after @Paul-Hebert tries it out.

Paul-Hebert commented 2 years ago

@calebeby running this now

Paul-Hebert commented 2 years ago

@calebeby dang, performance scores still seem way off on my machine: https://docs.google.com/spreadsheets/d/1b7nFD_42u49KsVd3X7mE6jY_oj8G9Hi_PwYp-LAb12k/edit#gid=1662529665

I used Node 14 if that matters? Am I meant to close other applications before running or anything like that?

calebeby commented 2 years ago

Thanks. Other programs running should be fine. Could you also check with node 16 since that's the most obvious difference between your machine and Scott's?

Paul-Hebert commented 2 years ago

Yep, I'll start that now

Paul-Hebert commented 2 years ago

@calebeby no dice. The scores I got running Node 16 were about the same as Node 14: https://docs.google.com/spreadsheets/d/1fVuiLvj9JfMeSGX-cItYzMYnr_wCIDSCNXaQJ26DHN0/edit#gid=1662529665

calebeby commented 2 years ago

@Paul-Hebert Thanks for running that. We might as well still go ahead with this PR even though it doesn't fix that issue.

spaceninja commented 2 years ago

Results 9.5.0 M1 Node 14

I had a spare minute so I went ahead and re-ran the test using Node 14

calebeby commented 2 years ago

Putting our names on the google sheets cuz it's getting hard to keep track.

@spaceninja It seems like you can reproduce the issue in node 14 but not node 16?

spaceninja commented 2 years ago

@calebeby yep. I just re-ran the test on 16 to confirm I wasn't getting different results, but my two node 16 runs match.

And my node 14 run on an M1 chip shows the problem results, while my node 16 runs on an M1 chip do not.

Paul-Hebert commented 2 years ago

Oh weird! Let me try re-running Node 16 again. These are the commands I used for that:

nvm use 16

lighthouse-parade https://cloudfour.com --max-crawl-depth 3 --exclude-path-glob "/thinks/*" --exclude-path-glob "/presents/*"

Do I need to do anything else? Should I re-run npm link after switching Node versions? Rebuild the project? Reinstall deps?

spaceninja commented 2 years ago

I don't know if it makes a difference, but I re-ran npm run build and npm link when I switched node verisons.

calebeby commented 2 years ago

Yeah there is a chance that you'll need to rerun npm link if you previously had a regularly globally installed version of lh-parade

Paul-Hebert commented 2 years ago

Okay, running the following:

nvm use 16
npm run build
npm link
lighthouse-parade https://cloudfour.com --max-crawl-depth 3 --exclude-path-glob "/thinks/*" --exclude-path-glob "/presents/*"
calebeby commented 2 years ago

Just to make sure the systems you're running on are the same, can you give the output of sysctl -n machdep.cpu.brand_string? Mine shows:

Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
spaceninja commented 2 years ago

If this turns out to be the problem, it's hard to ignore that node 16 was the first version that supported M1 chips. In order to run any previous version on an M1 chip, you have to install it via Rosetta.

spaceninja commented 2 years ago
$ sysctl -n machdep.cpu.brand_string
Apple M1
Paul-Hebert commented 2 years ago

Mine says:

Apple M1
Paul-Hebert commented 2 years ago

If this turns out to be the problem, it's hard to ignore that node 16 was the first version that supported M1 chips. In order to run any previous version on an M1 chip, you have to install it via Rosetta.

Can you expand on this? I don't think I did anything special for older versions of Node on my machine?

Paul-Hebert commented 2 years ago

Hmm, my results still seem off... https://docs.google.com/spreadsheets/d/1YW7j6lR6dfedOq6U01CQbqJxfJSTCbKasBag2vb6LAM/edit#gid=1662529665

calebeby commented 2 years ago

I am not sure completely how this works but what can you both run node -p process.arch in node 16 and then in node 14? My understanding is that node 14 will show x64 and node 16 should show arm64 if you are on m1 chips and it isn't doing virtualization

spaceninja commented 2 years ago

Can you expand on this?

Maybe something's changed, but when I got my M1 (which was pretty quickly after they came out), if you tried to install older versions of Node with nvm, you'd get compilation errors. I documented my experience in this Slack thread, but in a nutshell, I needed to open Terminal using Rosetta, install the older version of Node, and then I could go back to using Terminal without Rosetta and everything would run fine (basically, I only needed Rosetta emulation to compile the old version, once compiled, it would run fine).

Also, I was wrong, Node 15 is the first version with native M1 support.

spaceninja commented 2 years ago
$ nvm use 16
Now using node v16.14.0 (npm v8.3.1)

$ node -p process.arch
arm64

$ sysctl -n machdep.cpu.brand_string
Apple M1

$ nvm use 14
Now using node v14.16.0 (npm v6.14.11)

$ node -p process.arch
x64

$ sysctl -n machdep.cpu.brand_string
Apple M1
Paul-Hebert commented 2 years ago

I am not sure completely how this works but what can you both run node -p process.arch in node 16 and then in node 14? My understanding is that node 14 will show x64 and node 16 should show arm64 if you are on m1 chips and it isn't doing virtualization

Weird! This is what I see:

paulhebert@MacBook-Pro lighthouse-parade % nvm use 16
Now using node v16.14.0 (npm v8.3.1)
paulhebert@MacBook-Pro lighthouse-parade % node -p process.arch
x64
paulhebert@MacBook-Pro lighthouse-parade % nvm use 14
Now using node v14.18.0 (npm v6.14.15)
paulhebert@MacBook-Pro lighthouse-parade % node -p process.arch
x64
paulhebert@MacBook-Pro lighthouse-parade % 

Could this be related to the issues we're running into? I get incorrect results with both Node versions (in x64). @spaceninja only gets incorrect results when using v14 (which uses x64)

FWIW when I got my new machine I migrated over all the data from my old machine, so I never installed any new Node versions. (They were already present on my old machine)

calebeby commented 2 years ago

Yeah, this seems likely, your node 16 is using virtualized x64 instead of natively running arm instructions

calebeby commented 2 years ago

Can you maybe try these steps? https://www.jurnalanas.com/node-js-mac-m1/#switch-architecture

spaceninja commented 2 years ago

On the plus side, I think we can be fairly certain now that we have a fix for our M1 issue — we need to add a note to the readme saying that M1 users need to be running Node 15+ in arm64 mode.

Paul another thing to try is just reinstalling node 16:

nvm uninstall 16
nvm install 16
nvm use 16
node -p process.arch

That should reinstall the arm version.

spaceninja commented 2 years ago

Bonus: Once you get the arm version running, you'll notice Node 16 tasks are much faster now that they're running natively: https://cloudfour.slack.com/archives/C024G5KQV/p1626739576085500

Paul-Hebert commented 2 years ago

Nice! Trying this out now

Paul-Hebert commented 2 years ago

Yay, this is looking better!

https://docs.google.com/spreadsheets/d/15ljsZ3k1GmfZhkf4TqFWDSjiqgDEYDOG-Z6kHgPf9js/edit#gid=1662529665