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

Build in place on Windows during full rebuilds #11114

Merged
merged 2 commits into from Jul 9, 2020

Conversation

zodern
Copy link
Member

@zodern zodern commented Jul 8, 2020

One optimization Meteor uses during builds is build in place. When enabled, Meteor re-uses files from the previous build and only updates the files that changed. On Windows, this was disabled since there could be issues if the meteor tool updates a file the server is accessing. I enabled it for building the server in #10399 since the server is always stopped before a full rebuild is started. Since there hasn't been any issues, this PR enables it for the client architectures during full rebuilds.

Before, writing the client target would take between 600 and 950ms during rebuilds of a medium sized app on Windows.
After, it is between 70 and 215ms.

Building in place is still disabled for client only builds. Meteor 1.8 added the ability for the webapp to be paused during delayed builds. It might be possible to use this to make it safer to use in place builds, but it won't completely prevent issues.

@CaptainN
Copy link
Contributor

CaptainN commented Jul 8, 2020

What issues would be caused by writing to files that the server is accessing? Would it prevent the write, crash the server?

I'm partially familiar with the differences in the way Windows and NIX systems handle concurrent file access, but I'm not sure what the consequences would be in this case.

The follow up is, can the issue be handled on the back side in Windows (catch the access error, wait 500ms, then try again, or something) - so that it only slows down Windows rebuilds when this specific access problem occurs.

@zodern
Copy link
Member Author

zodern commented Jul 8, 2020

I'm not completely sure, but there are multiple comments in the meteor tool warning against it. Next time I look into this, I plan to try to cause this situation and see what happens. When build in place is disabled, Meteor deletes the folder containing the old files and replaces it with a new folder, and I've been wondering if that is any better than build in place.

@zodern
Copy link
Member Author

zodern commented Jul 8, 2020

This comment explains what their concerns were:

// using the old directory. The reason is that we don't want rebuilding to
// interfere with the running app, and we rely on the fact that on OS X and
// Linux, if the process has opened a file for reading, it retains the file
// by its inode, not path, so it is safe to write a new file to the same path
// (or delete the file).

@CaptainN
Copy link
Contributor

CaptainN commented Jul 8, 2020

Another idea would be to pause the running app during rebuild - anything to speed it up is preferable to the way it is now on Windows, IMHO.

@zodern
Copy link
Member Author

zodern commented Jul 8, 2020

I mentioned above one option for pausing the app:

Meteor 1.8 added the ability for the webapp to be paused during delayed builds. It might be possible to use this to make it safer to use in place builds, but it won't completely prevent issues.

Though I have several concerns:

  • Other packages access the files, such as dynamic import
  • What happens if a file was opened right before webapp was paused and is read while the file is replaced? This is unlikely if we pause it early enough in the build process.
  • Another option is to use a sync api (such as execSync) to completely pause the server, but I have the same question as in the previous item. This could also cause other issues if the client build takes long enough.

@CaptainN
Copy link
Contributor

CaptainN commented Jul 8, 2020

Doh! I read that, but it didn't compute. Darn dyslexia haha

@filipenevola filipenevola added this to the Release 1.10.3 milestone Jul 8, 2020
@filipenevola filipenevola changed the base branch from devel to release-1.10.3 Jul 8, 2020
@filipenevola filipenevola merged commit 94cd41f into release-1.10.3 Jul 9, 2020
20 checks passed
@filipenevola filipenevola deleted the full-rebuild-build-in-place branch Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants