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 system refactor #8140

Merged
merged 59 commits into from
Mar 9, 2020
Merged

Build system refactor #8140

merged 59 commits into from
Mar 9, 2020

Conversation

kumavis
Copy link
Member

@kumavis kumavis commented Feb 28, 2020

status: awaiting review

this is primarily a change to the "build/task system" and not the "bundling system"

change summary

  • build time performance improvements!
  • new colorful build timeline output 🌈
  • manifest platform overrides now specified as json files, located in app/manifest/*.json
  • renamed entry tasks to [prod, dev, testDev, test]. yarn script names unchanged for now.
  • dropped image compression from build (super slow)
  • build system broken into parts in development/build/*.js, gulpfile is gone
  • dropped gulp for task composition for simple async functions
  • dropped gulp for entry points. now just uses a simple js script to read entry from process.argv
  • renamed "copy" tasks -> "static"

plz QA: your usual workflow

@kumavis
Copy link
Member Author

kumavis commented Feb 28, 2020

added some candy

image

i find this timeline very helpful in identifying potential meaningful optimizations
eg even if you improve the speed of something, might not improve the time-to-completion bc everything is running parallel. but! its all running in a single node process so may still increase perf

@metamaskbot
Copy link
Collaborator

Builds ready [4ff258e]
Page Load Metrics (598 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint348747168
domContentLoaded31681259612761
load31881359812761
domInteractive31681159612761

@kumavis
Copy link
Member Author

kumavis commented Mar 2, 2020

experimenting with running each bundle in separate processes
results are local, may differ in CI due to processor count
bout a 2x speedup

before (single)

prod 155.0s
scripts:deps:background 112.9s
scripts:deps:ui 84.5s
scripts:core:prod:inpage 26.8s
scripts:core:prod:background 84.8s
scripts:core:prod:ui 151.7s
scripts:core:prod:phishing-detect 41.3s
scripts:core:prod:contentscript 2.4s

after (separate)

prod 75.4s
scripts:deps:background 70.3s
scripts:deps:ui 42.0s
scripts:core:prod:inpage 12.6s
scripts:core:prod:background 70.1s
scripts:core:prod:ui 70.1s
scripts:core:prod:phishing-detect 12.3s
scripts:core:prod:contentscript 4.5s

@kumavis
Copy link
Member Author

kumavis commented Mar 2, 2020

on CI prep-build 3m25s -> 2m29s

@kumavis kumavis mentioned this pull request Mar 7, 2020
package.json Outdated Show resolved Hide resolved
development/build/task.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [a1bdc8a]
Page Load Metrics (680 ± 34 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint36108612713
domContentLoaded5858476797234
load5878496807234
domInteractive5858476787234

package.json Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented Mar 8, 2020

I'm seeing this when a .scss file changes during a watch build:

(node:28142) UnhandledPromiseRejectionWarning: TypeError: stream.on is not a function
    at eos (/home/gudahtt/projects/metamask-extension/node_modules/end-of-stream/index.js:73:9)
    at eos (/home/gudahtt/projects/metamask-extension/node_modules/end-of-stream/index.js:14:41)
    at P (/home/gudahtt/projects/metamask-extension/node_modules/pify/index.js:49:6)
    at new Promise (<anonymous>)
    at /home/gudahtt/projects/metamask-extension/node_modules/pify/index.js:11:9
    at ret (/home/gudahtt/projects/metamask-extension/node_modules/pify/index.js:72:32)
    at watch (/home/gudahtt/projects/metamask-extension/development/build/styles.js:54:17)
    at write (/home/gudahtt/projects/metamask-extension/node_modules/gulp-watch/index.js:148:3)
    at /home/gudahtt/projects/metamask-extension/node_modules/gulp-watch/index.js:131:5

development/build/styles.js Outdated Show resolved Hide resolved
development/build/styles.js Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented Mar 8, 2020

I think this is ready to go after those last three changes. I've tested it out locally (with the above suggested changes) and it all seems to work

kumavis and others added 3 commits March 8, 2020 13:25
@kumavis
Copy link
Member Author

kumavis commented Mar 8, 2020

great catches, thanks

@metamaskbot
Copy link
Collaborator

Builds ready [132758f]
Page Load Metrics (643 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint359243126
domContentLoaded35576564110349
load35676764310249
domInteractive35576564110349

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@kumavis kumavis merged commit 7686eda into develop Mar 9, 2020
@whymarrh whymarrh deleted the build-sys-1 branch March 9, 2020 17:40
Gudahtt added a commit that referenced this pull request Jun 1, 2020
The version bump script referenced the old file path for the manifest.
It was stored as a single file called `manifest.json`, but it was split
into `_base.json` and platform-specific manifests in #8140.

The manifest version bump script has been updated to reference
`_base.json`, which is the piece that has the version property.
Gudahtt added a commit that referenced this pull request Jun 1, 2020
The version bump script referenced the old file path for the manifest.
It was stored as a single file called `manifest.json`, but it was split
into `_base.json` and platform-specific manifests in #8140.

The manifest version bump script has been updated to reference
`_base.json`, which is the piece that has the version property.
Gudahtt added a commit that referenced this pull request Feb 12, 2021
This package has not been used since #8140. We now spawn separate
processes directly in our build script rather than using this gulp
plugin to do so.
Gudahtt added a commit that referenced this pull request Feb 12, 2021
This package hasn't been used since #8140, which dropped it for being
too slow and of minimal benefit.

We should consider re-adding this as a CI check to ensure images are
optimized, but I don't think it should be re-added to the build process
itself.
@Gudahtt Gudahtt mentioned this pull request Feb 12, 2021
Gudahtt added a commit that referenced this pull request Feb 12, 2021
This package has not been used since #8140. We now spawn separate
processes directly in our build script rather than using this gulp
plugin to do so.
Gudahtt added a commit that referenced this pull request Feb 13, 2021
This package hasn't been used since #8140, which dropped it for being
too slow and of minimal benefit.

We should consider re-adding this as a CI check to ensure images are
optimized, but I don't think it should be re-added to the build process
itself.
Gudahtt added a commit that referenced this pull request Feb 15, 2021
This package hasn't been used since #8140, which dropped it for being
too slow and of minimal benefit.

We should consider re-adding this as a CI check to ensure images are
optimized, but I don't think it should be re-added to the build process
itself.
Gudahtt added a commit that referenced this pull request Feb 15, 2021
This package hasn't been used since #8140, which dropped it for being
too slow and of minimal benefit.

We should consider re-adding this as a CI check to ensure images are
optimized, but I don't think it should be re-added to the build process
itself.
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.

3 participants