Skip to content

Update lighthouse to 9.5.0 - #92

Merged
calebeby merged 2 commits into
masterfrom
lh-9-5-0
Mar 22, 2022
Merged

Update lighthouse to 9.5.0#92
calebeby merged 2 commits into
masterfrom
lh-9-5-0

Conversation

@calebeby

Copy link
Copy Markdown
Contributor

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

changeset-bot Bot commented Mar 15, 2022

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9f3c6ce

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

Copy link
Copy Markdown
Member

I'll test this out shortly

@spaceninja

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

spaceninja commented Mar 15, 2022

Copy link
Copy Markdown
Member

Results 9.5.0 M1 Node 16

@calebeby

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

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

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

@calebeby

Copy link
Copy Markdown
Contributor Author

@spaceninja were you using node 14?

@spaceninja

spaceninja commented Mar 15, 2022

Copy link
Copy Markdown
Member

Nope, I was running v16.14.0

Need me to re-run in 14?

@calebeby

Copy link
Copy Markdown
Contributor Author

Maybe after @Paul-Hebert tries it out.

@Paul-Hebert

Copy link
Copy Markdown

@calebeby running this now

@Paul-Hebert

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

Yep, I'll start that now

@Paul-Hebert

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

spaceninja commented Mar 17, 2022

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member
$ sysctl -n machdep.cpu.brand_string
Apple M1

@Paul-Hebert

Copy link
Copy Markdown

Mine says:

Apple M1

@Paul-Hebert

Copy link
Copy Markdown

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

Copy link
Copy Markdown

@calebeby

calebeby commented Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

spaceninja commented Mar 17, 2022

Copy link
Copy Markdown
Member
$ 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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

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

@calebeby

Copy link
Copy Markdown
Contributor Author

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

@spaceninja

spaceninja commented Mar 17, 2022

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

@spaceninja spaceninja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm approving, since at this point our discussion has shown the M1 issue is unrelated to this PR, which is still worth merging. Assuming Paul's testing confirms that M1 users need to make sure they're running the arm version of Node, I think a separate PR with a README update would be sufficient to address that.

@Paul-Hebert

Copy link
Copy Markdown

Nice! Trying this out now

@Paul-Hebert

Copy link
Copy Markdown

@calebeby
calebeby merged commit d0b1d12 into master Mar 22, 2022
@calebeby
calebeby deleted the lh-9-5-0 branch March 22, 2022 17:12
@github-actions github-actions Bot mentioned this pull request Mar 22, 2022
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