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

fix(watcher): native fsevents were not used in dist build on macOS #1104

Merged
merged 1 commit into from Aug 16, 2019

Conversation

@edvald
Copy link
Collaborator

commented Aug 15, 2019

What this PR does / why we need it:

This bug caused a tremendous slowdown and high CPU usage on Mac. The fix is
essentially to use a newer version of Pkg for macOS (but not for other
builds because the newer version breaks other platforms, yay), as well
as to update the fsevents binary.

I've also put in an assertion to try to prevent this happening again
without us noticing.

Fixes #1077

fix(watcher): native fsevents were not used in dist build on macOS
This caused a tremendous slowdown and high CPU usage on Mac. The fix is
essentially to use a newer version of Pkg for macOS (but not for other
builds because the newer version breaks other platforms, yay), as well
as to update the fsevents binary.

I've also put in an assertion to try to prevent this happening again
without us noticing.
@@ -82,6 +83,18 @@ export class Watcher {
this.log.debug(`Watcher: Watching paths ${this.paths.join(", ")}`)

if (watcher === undefined) {
// Make sure that fsevents works when we're on macOS. This has come up before without us noticing, which has
// a dramatic performance impact, so it's best if we simply throw here so that our tests catch such issues.
if (process.platform === "darwin") {

This comment has been minimized.

Copy link
@elliott-davis

elliott-davis Aug 15, 2019

I like this idea

@elliott-davis
Copy link

left a comment

holy moly - thanks for tracking this down. What an odd class of bug

tenor-192558074

@edvald

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 15, 2019

Lol, yeah that's pretty much how I felt when I saw it finally working. Odd class of bug indeed o_o.

# which unfortunately does NOT work for other platforms :/
echo " -> install pkg@4.4.0"
npm init -y
npm install pkg@4.4.0

This comment has been minimized.

Copy link
@eysi09

eysi09 Aug 16, 2019

Collaborator

Should we reset pkg to the package.json when this finishes so that the script leaves the node_modules as they were when we're running this locally? So that we don't run into issues when we test the script locally.

This comment has been minimized.

Copy link
@edvald

edvald Aug 16, 2019

Author Collaborator

This is performed in the dist directory, so it doesn't affect the garden-service package.

@eysi09
eysi09 approved these changes Aug 16, 2019

@eysi09 eysi09 merged commit 9fcdc45 into master Aug 16, 2019

1 check passed

commit Workflow: commit
Details
@edvald

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

We should make sure to test the CI generated edge binaries once they're built. For reference, I could reproduce the reported CPU usage issue by simply running Garden in a project with a large number of files (e.g. with a fairly large node_modules folder).

@eysi09 eysi09 deleted the fix-mac-dist-watcher branch Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.