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

Allow to exclude web architectures in development mode #10824

Merged
merged 3 commits into from Jan 13, 2020

Conversation

sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Dec 18, 2019

This commit implements an --exclude-archs arg to the run command which allows you to pass a comma-seperated list of architectures to exclude.

Motivation:
Improve rebuild speeds, by allowing to leave out certain web architectures during development.

Meteor rebuild speeds, although greatly improved in recent versions, is still a common point of frustration as seen in the issues (#10800) and on the forum (https://forums.meteor.com/t/2x-to-3x-longer-build-time-also-on-testing-since-1-8-2-update/51028/3)

One reason for slow rebuilds is high memory pressure (#9568).
The web.browser.legacy architecture is currently always being build while most of us will only test this every once in a while.
So while we don't need it most of times during development it does add to the high memory usage.
Furthermore, even though these builds are delayed, long-running synchronous tasks have the potential to block the server startup (#10261).
This last issue can be partially, but not fully, improved by #10427
Therefore a commonly requested feature has been to disable the legacy build (meteor/meteor-feature-requests#333)

However, we may also temporarily disable cordova builds and we may even want to disable just the web.browser build if we're just debugging a bug in the legacy build.
Therefore I chose to create the more generic --exclude-archs option.

@sebakerckhof
Copy link
Contributor Author

@sebakerckhof sebakerckhof commented Dec 18, 2019

ping @veered

Copy link
Member

@filipenevola filipenevola left a comment

Great work! thanks

@RobertLowe
Copy link
Contributor

@RobertLowe RobertLowe commented Dec 20, 2019

any METEOR_PROFILE=1s to compare?

@sebakerckhof
Copy link
Contributor Author

@sebakerckhof sebakerckhof commented Dec 20, 2019

@RobertLowe Here's 2 profiles from a cold start:

182-standard-cold.txt
182-nolegacy-cold.txt

During development/rebuilds it's using 1/3rd less memory (about 1gb for our application). In case you're low on RAM, this will help for rebuild times as well.

@RobertLowe
Copy link
Contributor

@RobertLowe RobertLowe commented Dec 20, 2019

@sebakerckhof nice, I see about 100s saved build time, very nice! Curious, did you clean build caches between?

@sebakerckhof
Copy link
Contributor Author

@sebakerckhof sebakerckhof commented Dec 20, 2019

@RobertLowe yes both were after a meteor rebuild which clears the caches.
Anyway, the metric for a cold boot isn't very interesting IMO, since you don't do that often. I'm way more interested in rebuild times. And although the legacy rebuild is delayed, the additional memory usage for the legacy build can really slow down the modern rebuild if you're short on memory.

@RobertLowe
Copy link
Contributor

@RobertLowe RobertLowe commented Dec 20, 2019

@sebakerckhof it's still a welcome improvement!! Thanks for getting this out there, thanks @benjamn for targeting this for 1.8.4!! 👍 🎉

@filipenevola filipenevola changed the base branch from devel to release-1.8.4 Dec 23, 2019
@Gywem Gywem mentioned this pull request Jan 2, 2020
@hexsprite
Copy link
Contributor

@hexsprite hexsprite commented Jan 2, 2020

Will this work for running tests as well? I noticed that they can also build archs that I don't care about...

@sebakerckhof
Copy link
Contributor Author

@sebakerckhof sebakerckhof commented Jan 7, 2020

@hexsprite I've added the option for tests as well. Should also address #10690

@sebakerckhof
Copy link
Contributor Author

@sebakerckhof sebakerckhof commented Jan 7, 2020

Btw, currently this will crash if you do something like:
meteor run android --exclude-archs web.cordova
Because well, this doesn't make much sense, so we can handle it in multiple ways:

  1. Ignore the exclude archs for web.cordova
  2. Don't start the cordova runner, just start the server
  3. Throw an error saying this combination is invalid

Any preferences to what seem best?

@hexsprite
Copy link
Contributor

@hexsprite hexsprite commented Jan 7, 2020

testing this out when running tests. sure makes a huge difference in build times and memory pressure as you say!

Copy link
Member

@benjamn benjamn left a comment

This looks great to me!

@sebakerckhof Can you rebase against devel? I think this might make more sense for a Meteor 1.9.1 release, now that Meteor 1.9 has been finalized.

@RobertLowe
Copy link
Contributor

@RobertLowe RobertLowe commented Jan 10, 2020

@benjamn 1.9.1-rc.0 eta? 😛 this feature is significantly helpful

@sebakerckhof sebakerckhof changed the base branch from release-1.8.4 to devel Jan 13, 2020
@benjamn benjamn force-pushed the feat/exclude-webarchs branch from 03605dc to 2157f39 Compare Jan 13, 2020
sebakerckhof and others added 3 commits Jan 13, 2020
This commit implements an --exclude-archs arg to the run command
which allows you to pass a comma-seperated list of architectures
to exclude.

Motivation:
Improve rebuild speeds, by allowing to leave out certain
web architectures during development.

Meteor rebuild speeds, although greatly improved in recent versions,
is still a common point of frustration as seen in the issues (meteor#10800)
and on the forum (https://forums.meteor.com/t/2x-to-3x-longer-build-time-also-on-testing-since-1-8-2-update/51028/3)

One reason for slow rebuilds is high memory pressure (meteor#9568).
The web.browser.legacy architecture is currently always being build
while most of us will only test this every once in a while.
So while we don't need it most of times during development
it does add to the high memory usage.
Furthermore, even though these builds are delayed,
long-running synchronous tasks have the potential to block the server startup (meteor#10261).
This last issue can be partially, but not fully, improved by meteor#10427
Therefore a commonly requested feature has been to disable the legacy build (meteor/meteor-feature-requests#333)

However, we may also temporarily disable cordova builds
and we may even want to disable just the web.browser build if we're just debugging
a bug in the legacy build.
Therefore I chose to create the more generic --exclude-archs option.
@benjamn benjamn force-pushed the feat/exclude-webarchs branch from 2157f39 to 54b6803 Compare Jan 13, 2020
@benjamn benjamn removed this from the Release 1.8.4 milestone Jan 13, 2020
@benjamn benjamn added this to the Release 1.10 milestone Jan 13, 2020
@benjamn benjamn merged commit a205967 into meteor:devel Jan 13, 2020
17 of 18 checks passed
@benjamn benjamn mentioned this pull request Jan 14, 2020
5 tasks
benjamn added a commit that referenced this issue Jan 16, 2020
We accidentally published changes to webapp that should have been
restricted to Meteor 1.10 as part of the 1.8.1 version. This commit
reverts commits to packages/webapp since Meteor 1.9, so that we can
republish the 1.8.0 content as version 1.8.2. We will then bump the webapp
version to 1.9.0 on the release-1.10 branch and publish the new content
only on that branch.

Revert "Allow to exclude web architectures in development mode (#10824)"
This reverts commit a205967.

Revert "Updates cordova-plugin-meteor-webapp to 1.7.1"
This reverts commit a1e4d27.

Revert "Update cordova-plugin-wkwebview-engine to 1.2.1."
This reverts commit 3f9a69d.

Revert "Update cordova-plugin-whitelist to 1.3.4."
This reverts commit 9792733.

Revert "Update cordova-plugin-meteor-webapp to 1.7.1-beta.1."
This reverts commit 565c425.

Revert "Update accounts-password to version 1.5.2."
This reverts commit b827d1d.
benjamn added a commit that referenced this issue Mar 30, 2020
Follow-up to #10824.

If the preferred arch is not available (most likely because it was
deliberately excluded), it's better to use another client arch that is
available than to guarantee the site won't work by returning an unknown
arch. For example, if web.browser.legacy is excluded using the
--exclude-archs option (introduced by #10824), legacy clients are better
served by receiving web.browser (which might actually work) than receiving
an HTTP 404 response. If none of the arches in preferredArchOrder are
defined, only then should we send a 404.
benjamn added a commit that referenced this issue Mar 31, 2020
Follow-up to #10824.

If the preferred arch is not available (most likely because it was
deliberately excluded), it's better to use another client arch that is
available than to guarantee the site won't work by returning an unknown
arch. For example, if web.browser.legacy is excluded using the
--exclude-archs option (introduced by #10824), legacy clients are better
served by receiving web.browser (which might actually work) than receiving
an HTTP 404 response. If none of the arches in preferredArchOrder are
defined, only then should we send a 404.
@dosomder
Copy link

@dosomder dosomder commented Apr 7, 2020

@sebakerckhof Why only for run / test? Would be useful to disable legacy with build too

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.

None yet

6 participants