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

Bundler Refactor #8170

Closed
wants to merge 26 commits into from
Closed

Bundler Refactor #8170

wants to merge 26 commits into from

Conversation

kumavis
Copy link
Member

@kumavis kumavis commented Mar 7, 2020

status: ready for additional feedback or merge 🎸

refactor of the bundler using lavamoat-friendly bundle factoring. this does not add lavamoat to the bundles.

changes to bundle result

  • the ui-libs and bg-libs have been replaced by the common bundle. common contains all packages that are used by both the background and the ui. the "factoring" into the common bundle is automatic and does not need any adjustment as modules are added/removed.

here is a breakdown of the resultant bundle sizes under different minification strategies. the current configuration is the strategy all the way to the right. compare the size of bg/ui/common.
image

changes to bundling process

I tried to refactor the bundler so that it was easier to follow without having too much repeated code, but im not sure that I succeeded. part of what complicates this is that certain things (e.g. sourcemaps) need to configure both:

  1. initial bundle options (enable browserify debug option) and
  2. post-bundle build pipeline (load + write sourcemaps)

its additionally complicated because a watchify re-build involves creating a new build pipeline. my proposed solution is as follows

a basic bundle+build config obj

{ bundlerOpts: {}, events: new EventEmitter() }

events will fire a 'pipeline' event when the build pipeline is ready to be augmented. The pipeline is based on browserify's own internal pipeline, a labeled-stream-splicer. It is essentially a nested array of streams that you can pop into position, and it handles all the plumbing for you. it may be slightly over-engineered, but it allows our very different build configurations (inpage+contentscript standalone, ui+background factor) share the same interface, and makes applying some feature (e.g. sourcemaps) able to happen in one place

etc

moved the lavamoat dashboard (sesify-viz) to a separate process that runs parallel with the other builds, should speed up CI ~3min

@kumavis kumavis changed the base branch from develop to build-sys-1 March 7, 2020 13:37
@metamaskbot
Copy link
Collaborator

Builds ready [915c7c8]
Page Load Metrics (629 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint34135472210
domContentLoaded4567396276029
load4577406296029
domInteractive4557396276029

@kumavis kumavis changed the base branch from build-sys-1 to develop March 9, 2020 00:55
@metamaskbot
Copy link
Collaborator

Builds ready [aaedd5f]
Page Load Metrics (621 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint36524042
domContentLoaded4357736208541
load4367756218541
domInteractive4347736208541

@metamaskbot
Copy link
Collaborator

Builds ready [b43fc43]
Page Load Metrics (590 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint34463731
domContentLoaded2906525889043
load2936545909043
domInteractive2906515889043

@metamaskbot
Copy link
Collaborator

Builds ready [ce9c9aa]
Page Load Metrics (688 ± 31 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint34189534019
domContentLoaded6078446866431
load6098476886531
domInteractive6078446866431

@metamaskbot
Copy link
Collaborator

Builds ready [89a1802]
Page Load Metrics (871 ± 92 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint42184824723
domContentLoaded352141686819192
load353142087119292
domInteractive351141586819192

@metamaskbot
Copy link
Collaborator

Builds ready [f8b0107]
Page Load Metrics (703 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint35140532512
domContentLoaded447105370013967
load455105570313967
domInteractive446105369913967

@kumavis

This comment has been minimized.

@kumavis

This comment has been minimized.

@metamaskbot
Copy link
Collaborator

Builds ready [fa96846]
Page Load Metrics (646 ± 11 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint35543942
domContentLoaded6126996442411
load6147016462411
domInteractive6126986442411

* origin/develop: (582 commits)
  Use async/await for seedPhraseVerifier.verifyAccounts (#9100)
  Use async/await for getRestrictedMethods (#9099)
  Update dependencies (#9105)
  update email us to contact us (#9104)
  Improve source maps (#9101)
  Update font family globally (#9073)
  rpc-cap@3.1.0 (#9103)
  Use environment variable for production Sentry DSN (#9097)
  Only log error on first occurrence of missing substitution (#9096)
  Use mixins for typography instead of placeholder selectors (#9072)
  Update css folder structure (#9071)
  Disable Sentry in development (#9095)
  Use environment variable for MetaMetrics project ID (#9094)
  Use development metametrics project during tests (#9093)
  json-rpc-engine@5.2.0 (#9091)
  fixup! call initializeProvider where necessary
  call initializeProvider where necessary
  Add euclid fontface (#9018)
  fix timing-reliant network controller test
  Robustify permissions controller requestUserApproval tests (#9064)
  ...
@kumavis kumavis requested a review from a team as a code owner July 30, 2020 15:32
@metamaskbot
Copy link
Collaborator

Builds ready [1e042f2]
Page Load Metrics (502 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint2812240199
domContentLoaded3536465008340
load3546475028340
domInteractive3526455008340

@Gudahtt
Copy link
Member

Gudahtt commented Jul 30, 2020

The bundles produced by this branch look good, but the sourcemaps for common.js are totally wrong 🤔

I'll try and reduce the size of this PR first by getting a few of these changes merged separately, then I'll investigate that problem.

Edit: Done. This PR is as small as it'll get for now. It might still be able to be split between the build changes and the new lavamoat build step; TBD.

Gudahtt added a commit that referenced this pull request Jul 30, 2020
There are no functional changes. This was extracted from #8170
Gudahtt added a commit that referenced this pull request Jul 30, 2020
There are no functional changes. This was extracted from #8170
* origin/develop:
  Factor out `getEnvironment` function in build script (#9114)
  Update `browserify` from v16.2.3 to v16.5.1 (#9113)
  Update `sesify-viz` from v3.0.9 to v3.0.10 (#9111)
  Update `gulp-rename` from v1.4.0 to v2.0.0 (#9112)
  Update `source-map-explorer` from v2.0.1 to v2.4.2 (#9110)
  Add `build-artifacts` to .gitignore (#9109)
`browserify-transform-tools` is now used indirectly via
`lavamoat-browserify`, which has also replaced `sesify`.
@metamaskbot
Copy link
Collaborator

Builds ready [4b77117]
Page Load Metrics (514 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27135402211
domContentLoaded4036795129545
load4046805149545
domInteractive4036785119545

* origin/develop:
  Update `brfs` from v1.6.1 to v2.0.2 (#9115)
@Gudahtt
Copy link
Member

Gudahtt commented Jul 30, 2020

It looks like the source maps for all of the bundles are messed up. The sources entry in ui.js.map lists a bunch of background modules, for example.

Edit: It turns out that just the three "factored" bundles are affected (ui.js, background.js, and common.js).

@metamaskbot
Copy link
Collaborator

Builds ready [8b8bce8]
Page Load Metrics (421 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26403232
domContentLoaded25363642010852
load25563842110852
domInteractive25363641910852

@Gudahtt
Copy link
Member

Gudahtt commented Aug 4, 2020

I tried looking at the direct output of bify-package-factor to check the inline source maps, and they seem correct 🤔 Or at least they weren't as obviously incorrect as the final output. So maybe the problem here is with something we're doing after bify-package-factor.

* origin/develop:
  Fix connection removal bug (#9137)
  Add source map validator to CI (#9135)
  Update source map validator target files (#9133)
  Improve sourcemap validator console report (#9131)
  Add `validate-source-maps` npm script (#9134)
  Non-zero exit code upon failure to validate source maps (#9132)
  remove unused tx-list styles (#9121)
  delete unused confirm styles (#9118)
@Gudahtt
Copy link
Member

Gudahtt commented Aug 5, 2020

I've just merged develop into this again to bring in the source map validator script changes (and I updated it to check the common.js bundle), so CI should now fail until the source maps are fixed.

Gudahtt added a commit that referenced this pull request Aug 7, 2020
There are no functional changes. This was extracted from #8170
@kumavis
Copy link
Member Author

kumavis commented Jan 18, 2021

this PR is super stale now, but has some potentially useful stuff

@kumavis
Copy link
Member Author

kumavis commented Jan 18, 2021

i want to revert this to a draft pr but i dont see an option for that?

@brad-decker
Copy link
Contributor

Screen Shot 2021-01-18 at 9 53 50 AM

I didn't know this existed until Whymarrh pointed it out to me. Paying it forward, also marking this as draft.

@kumavis
Copy link
Member Author

kumavis commented Apr 27, 2021

rip

@kumavis kumavis closed this Apr 27, 2021
@kumavis kumavis deleted the build-sys-2 branch April 27, 2021 23:50
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants