Skip to content

Parallelize feature spec build, leverage Make up-to-date for browsers.json#9959

Merged
aduth merged 2 commits intomainfrom
aduth-parallel-build
Feb 15, 2024
Merged

Parallelize feature spec build, leverage Make up-to-date for browsers.json#9959
aduth merged 2 commits intomainfrom
aduth-parallel-build

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Jan 23, 2024

🛠 Summary of changes

Optimizes a few tasks surrounding JavaScript build:

  • Runs JavaScript and stylesheet asset compilation in parallel for feature spec pre-build
  • Runs package.json build (now build:js) subtasks in parallel
  • Takes advantage of Makefile "up to date" to avoid unnecessary browsers.json build
    • Note that yarn generate-browsers-json is already pretty fast (0.22s on average) so the room for optimization is minimal, but an up-to-date make browsers.json is faster (0.065s), and the approach allows for some tolerance if ever the work involved in generate-browsers-json became more involved / time-consuming.

Performance Results

Diff:

diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index cd31bb0e6..05750c1d3 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -80,4 +80,7 @@ RSpec.configure do |config|
       print '                       Bundling JavaScript and stylesheets... '
-      system 'WEBPACK_PORT= yarn build > /dev/null 2>&1'
-      system 'yarn build:css > /dev/null 2>&1'
+      time = Benchmark.measure do
+        system 'WEBPACK_PORT= yarn build > /dev/null 2>&1'
+        system 'yarn build:css > /dev/null 2>&1'
+      end
+      puts time.real
       puts '✨ Done!'

Average of 5 runs: rspec spec/features/accessibility/user_pages_spec.rb:9

Before: 6.66s
After: 4.34s
Diff: -2.32s (-34.8%)

Average of 10 runs: yarn build (yarn build:js)

Before: 3.00s
After: 2.90s
Diff: -0.1s (-3.33%)

📜 Testing Plan

Verify feature specs generate JavaScript and stylesheets:

  1. rm -rf public/packs app/assets/builds/*.css
  2. rspec spec/features/accessibility/user_pages_spec.rb:9

Verify build command completes successfully:

  1. yarn build:js

Observe in repeated calls to yarn build:js that make skips unnecessary work

  1. yarn build:js
  2. yarn build:js
  3. Observe in output: "make: `browsers.json' is up to date."

package.json Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just for my own edification, in other project's I've seen you use run-p so we 'd have like:

Suggested change
"build:js": "concurrently 'yarn:webpack' 'make browsers.json'",
"build:js": "run-p build:js:*",
"build:js:webpack": "yarn:webpack",
"build:js:browsers-json": "make browsers.json",

is there a reason we didn't do that here? to me seems like adding a new dependency either way so idk if one is better/preferred/more common than the other

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I recall being faced with this choice with some recent transitive dependency having a vulnerability, and discovering that npm-run-all (run-p) hasn't received a new release in 5 years, which was a motivator to find a maintained alternative. concurrently also makes it a little easier to work with non-NPM commands like what's used here.

But ultimately, yes, they're largely used to the same end, and I think your proposed alternative could work just as well.

changelog: Internal, Build Tooling, Improve performance of JavaScript build
@aduth aduth force-pushed the aduth-parallel-build branch from c3d1c37 to e907ae6 Compare February 15, 2024 14:49
@aduth aduth merged commit e9504f7 into main Feb 15, 2024
@aduth aduth deleted the aduth-parallel-build branch February 15, 2024 18:45
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.

2 participants