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

Write performance #10454

Merged
merged 6 commits into from
Feb 28, 2019
Merged

Write performance #10454

merged 6 commits into from
Feb 28, 2019

Conversation

zodern
Copy link
Collaborator

@zodern zodern commented Feb 14, 2019

JS minify performance
In development, most of the time spent minifying is from converting the content of the files to and from Buffers. This was done before minifying for dynamic files to rename the function, and afterwards for all files to restore the function name. These changes have it avoid converting the contents to and from buffers where possible, and only restores the function name for dynamic files. Saves ~400ms per rebuild in a larger app.

Skip calculating SRI for assets
The current SRI standard supports css and js files, and Meteor only uses it for the main css and js bundles. These changes disable calculating the SRI hash for assets since Meteor currently doesn't use it for assets. If some packages or apps want the SRI hash for js or css assets, then two alternatives are calculating it for production, or skipping it for assets that are not js and css files. Saves ~700ms per rebuild in one app.

Skip removing source map comments if the content won't be written
Before writing a file, Meteor removes source map comments that are in the middle of the file. These changes have it skip that step when the file's contents didn't change and it is building in place. Saved 300ms during rebuilds.

@KoenLav
Copy link
Contributor

KoenLav commented Feb 14, 2019

@zodern I think I'm speaking for a lot of us here when I say: thank you for all the efforts on build time improvements!

I guess the times mentioned here, and in your other recent PRs, are improvements even on top of the changes already implemented in the Meteor 1.8.1 beta?

@zodern
Copy link
Collaborator Author

zodern commented Feb 15, 2019

Thanks @KoenLav. Yes, the times are all additional, though they are from a larger app so most apps will probably not see as large of an improvement.

@smeijer
Copy link

smeijer commented Feb 18, 2019

Couldn't agree more with @KoenLav, I wanted to say the same thing in #10453.

@zodern, thanks for all the time and energy you're putting into this!

Now give this guy some merge permissions! 😄

zodern and others added 6 commits February 28, 2019 14:02
Saves on average ~2ms per file in one app, which adds up when there are
hundreds. SRI is currently only supported for js and css files, and Meteor
only uses it for the main bundles.
Avoids converting file contents to and from buffers and strings. The
conversion had been done for dynamic files before minifying them, and all
files after they were minified.
Copy link
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Just rebased to see if the tests will pass now, but otherwise this looks great!

@benjamn benjamn merged this pull request into meteor:release-1.8.1 Feb 28, 2019
benjamn pushed a commit that referenced this pull request Feb 28, 2019
@benjamn
Copy link
Contributor

benjamn commented Feb 28, 2019

@zodern Happy to add you as a collaborator with push/edit permissions, by the way. Just let me know if that sounds useful to you.

@zodern
Copy link
Collaborator Author

zodern commented Mar 2, 2019

@benjamn I think that would be useful, thanks. I sent you a message on the forums with a couple of questions.

benjamn pushed a commit that referenced this pull request Mar 9, 2019
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
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.
@@ -619,7 +625,9 @@ class File {
hashes.push(this._inputHash);
}

hashes.push(this.sri());
if (!this._skipSri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is OK. Because of this change, now effectively when _skipSri is set, contents of the file are not part of the hash anymore.

@@ -628,7 +636,7 @@ class File {
}

sri() {
if (! this._sri) {
if (! this._sri && !this._skipSri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong place to put this. I think if you call sri(), sha512 should always be computed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should wrap manifestItem.sri = file.sri() with _skipSri check.

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.

5 participants