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

Windows build performance #10399

Merged
merged 18 commits into from Jan 31, 2019

Conversation

Projects
None yet
7 participants
@zodern
Copy link
Collaborator

commented Jan 1, 2019

Before:

The times are from the todos app. Rebuilds are for editing a file that is only used in the client, but not in a client folder, causing both the server and clients to be rebuilt.

Initial build (second and later runs):

resolveContraints: 2,149 2,261
prepareProjectForBuild: 10,299 11,230
build app: 20,656 18,792
App process started after: 36,902 35,839
Creating watchers finished: 12,383 13,462ms after app started
AppProcess onListen: 23,900 25,644ms after app started

Usuable after: 60,802 61,483

Rebuild

prepareProjectForBuild: 6,713 6,674 7,158
rebuildApp: 4,555 4,269 4,008
App process started after: 11,690 11,499 11,621
Creating watchers finished: 4,536 4,057 4,001ms after app started
AppProcess onListen: 8,746 8,090 8,015ms after app started
Delayed build: 9,469 8,484 8,406ms after app started

Usuable after: 20,436 19,589 19,636
Delayed build at: 21,159 19,983 20,027

After:

Initial build (second and later runs):

resolveContraints: 1,392 1,879 2,053
prepareProjectForBuild: 6,601 7,267 6,708
build app: 16,056 14,270 12,810
App process started after: 28,662 26,679 24,557
AppProcess onListen: 2,007 2,214 1,593 after app started
Watchers are created after onListen and are no longer blocking

Usuable after: 30,669 28,893 26,150

Rebuild

prepareProjectForBuild: 6,187 6,013 6,359
rebuildApp: 2,587 2,297 1,842
App process started after: 9,256 8,757 8,811
AppProcess onListen: 1,936 1,894 1,697 after app started
Delayed build: 6,255 6,522 6,551 after app started

Usuable after: 11,192 10,651 10,508
Usuable + delayed build: 15,511 15,279 15,362

Notable Changes

The large app times are from the rebuild profile at #5091 (comment).

Delete garbage directories async

Saves 500 - 1,100ms per rebuild (large app 9s)

Do not use atomicallyRewriteFile when not building in place

Saves ~650ms (large app ~29,000ms including legacy bundle) per build.
This should be safe since the build is only used once all files have been successfully written. Renaming the two files takes almost twice as long as writing it.

isPortable

Saves ~900ms. Since npm-rebuilds.json is never used from the bundle created by "meteor run", creating it's contents is skipped.
The .meteor-portable file to cache the results was never created on Windows due to the file path being in the wrong format, which slowed down the initial build and meteor build when using local packages.

inPlace server builds on windows and caching _copyDirectory

Saves 1 - 1.5s per rebuild. Most of the warnings against doing this on windows give the reason of interfering with the running app. Since the server is always stopped during the build, it should be safe to update it in place.

If it is safe to update the server in place, it might also be safe to do the same with the legacy bundle since the server doesn't serve it while it is being written.

Create file watcher and start legacy bundle after server started

Both of those were blocking the event loop, causing a delay between the server starting, and the tool process logging '=> server started' and letting the proxy know to forward requests.

Watcher now has an option to do the initial check async. The delayed legacy bundle blocking has not been fixed.

zodern added some commits Jan 1, 2019

Normalize paths before comparing them in symlinkWithOverwrite
In windows, source is in posix format and missing a trailing slash, causing the symlink to always be recreated.
Do not calculate rebuild dirs in "meteor run"
It is never used in the bundles created by "meteor run", and adds 0.5 - 1.5 seconds per build for smaller apps.
Use in place builds on windows for the server
Since the server is always stopped when building the server, it should be safe to overwrite it's files.
Add caching to _copyDirectory
This is most noticible in copyNodeModulesDirectory, which is called many times during each server build.
Fix caching meteorNpm.isPortable on windows
The path was in the wrong format, so .meteor-portable.json was never saved.
Fix delay between server starting and showing "=> Server restarted"
Creating the watcher can take up to 12+ seconds in small - medium apps, and uses sync fs calls.
The server would start right away, but the tool process wouldn't know about it until the watcher finished setting up. Also, the proxy doesn't forward requests until "=> Server restarted" is shown.
A new async option is added to Watcher which prevents it from blocking the event loop too long.
Also, the watcher and legacy bundle are only created after the server has started, or 3 seconds has passed.
Fix files.readdir and files.realpath not being cached
enableCache was called before readdir and realpath was added to "files".
Reduce duplicate fs calls in watcher
Most directories are in the WatchSet at least twice, and the directory is read and each item is stat each time. In addition, when the watcher is not created by isUpToDate, each item in watchSet.directories and watchSet.files is checked twice.

With these changes, isUpToDate finishes in less than 1/2 the time on Windows, and creating a watcher takes around 1/4 the time.
@zodern

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 5, 2019

After the last 4 comments, the build now is:

Initial

resolveConstraints: same
prepareProjectForBuild: 3,827 3,575 3,667
build app: 13,345 14,427 12,939
App process started after: 23,302 23,539 22,074
AppProcess onListen: same
Usuable after: 24,928

Rebuild client and server

prepareProjectForBuild: 1,762 797 958
rebuildApp: 2,333 1,817 1,723
App process started after: 4,485 2,915 2,999
Listened: same
Delayed build: 3,998 3,025 3,197ms after app started

Usable after 5,579
Usable + legacy: 9,577

Client only rebuild before last 4 commits

onChange: 2,562 3,389 3,136
prepareProjectForBuild: 4,444 5,029 3,542
rebuildApp: 1,580 1,438 1,180
Legacy built in 3,854 3,412 3,3842

Usable after 8,440

Client only rebuild after last 4 commits

onChange: 1,162 1,108 1,048
prepareProjectForBuild: 2,011 797 725
rebuildApp: 1,731 1,649 1,600
legacy built in: 1,365 1,129 1,144
Usable after 4,777 (first rebuild) 3,563 (subsequent rebuilds)

The client only rebuilds are timed before the server is rebuilt for the first time.

After the initial build, the watch sets seems to include every folder and some files in node_modules. The watch set created after a rebuild is missing those. This does make onChange very fast though, so causing the server to rebuild once after the app is started might make client rebuilds a couple seconds faster.

@CaptainN

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

In my limited local testing, this PR makes rebuilds FASTER under Windows than under a Linux VM on the same machine!

@CaptainN

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

fast-windows-meteor-rebuild.zip
fast-windows-meteor-startup.zip

A quick stop watch test shows 23 seconds rebuild, vs 24 seconds in a VM (save to browser response). Much improved over the 80-100 (or more) seconds it used to take.

@yorrd

This comment has been minimized.

Copy link

commented Jan 12, 2019

Since this is not targeted at Linux, I didn't expect huge gains, but I can confirm that rebuild times are within variance and didn't change on Linux. I'd like to do some more accurate measurements but GC is getting in my way at the moment

@smeijer

This comment has been minimized.

Copy link

commented Jan 12, 2019

I'm having build times of more than 2 minutes. This patch would be more very, very welcome!

@KoenLav KoenLav referenced this pull request Jan 14, 2019

Merged

Release 1.8.1 #10248

@benjamn benjamn force-pushed the meteor:release-1.8.1 branch from 5e26ebb to adaf653 Jan 15, 2019

@benjamn benjamn added this to the Release 1.8.1 milestone Jan 15, 2019

@benjamn

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

I haven't had time to give this PR a thorough review yet, but we will definitely be including this PR in Meteor 1.8.1, in case there was any doubt.

@hwillson hwillson self-requested a review Jan 16, 2019

@hwillson
Copy link
Member

left a comment

Thanks very much for working on this @zodern! This looks awesome and your benchmarking results are amazing! Tool changes like these can definitely be tricky, so we'll plan on getting these changes into a published beta release, to let people try them out. Thanks again for your work here!

Show resolved Hide resolved tools/isobuild/builder.js
Show resolved Hide resolved tools/runners/run-app.js Outdated
@CaptainN

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

Something is causing the spinner to remain after it is finished spinning:

=> Meteor server restarted                   -
I20190117-23:17:18.841(-5)? http://localhost:3000/
I20190117-23:17:18.863(-5)? http://localhost:3000/
I20190117-23:17:18.873(-5)? http://localhost:3000/
=> Meteor server restarted                   |
=> Meteor server restarted                   -
=> Meteor server restarted                   |
=> Meteor server restarted                   /
@CaptainN

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

It seems that removing that 3 second race might have added back the lengthy end of build delay. I'll do more testing with and without and see if it reveals anything.

@CaptainN

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

I just have to say, this is so fast, and makes Meteor on Windows such a pleasure to use. I can't wait until this is merged. I haven't noticed any delay after rebuild - either there isn't one, or it's just so fast I can't tell. Awesome work!

@smeijer

This comment has been minimized.

Copy link

commented Jan 31, 2019

Is this branch present in the current beta release?

@benjamn
Copy link
Member

left a comment

Let's go ahead and get this into a beta build so folks can use it. None of my feedback is worth delaying the beta release.

@benjamn benjamn merged commit 419ff7c into meteor:release-1.8.1 Jan 31, 2019

18 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tabasco-addict

This comment has been minimized.

Copy link

commented Jan 31, 2019

To use the newest release, do i install normally using choco , then replace the files in the .meteor folder with the ones in the 1.8.1 repo?

@benjamn

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

@tabasco-addict Install via choco (which will give you Meteor 1.8.0.2) and then run

meteor update --release 1.8.1-beta.16

in any application directory. You can also create a new app with

meteor create --release 1.8.1-beta.16 new-181b16-app
@smeijer

This comment has been minimized.

Copy link

commented Feb 1, 2019

Okay, this one is officially awesome! Thanks @zodern and @benjamn!

My build time (hot update) went down from 42 seconds to 9 seconds. Waiting for the browser refresh, takes another 5 seconds.

Usually, Meteor starts to be slower after a couple of hours development with a number of rebuilds. So I still need to monitor that one. But I have good hope.

I've installed Ubuntu 2 days ago. Turns out, that that was either a year too late, or a day to soon.

benjamn added a commit that referenced this pull request Mar 9, 2019

Update CONTRIBUTING.md, adding @zodern as a collaborator.
Adding @zodern as a collaborator with write access (including
triage/review), based on contributions that demonstrate deep understanding
of the meteor/meteor codebase: #9887, #10399, #10452, #10453, #10454

Also updated other parts of CHANGELOG.md to reflect 2019 realities.

benjamn added a commit that referenced this pull request Mar 13, 2019

Update CONTRIBUTING.md, adding @zodern as a collaborator. (#10485)
Adding @zodern as a collaborator with write access (including
triage/review), based on contributions that demonstrate deep understanding
of the meteor/meteor codebase: #9887, #10399, #10452, #10453, #10454

Also updated other parts of CHANGELOG.md to reflect 2019 realities.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.