On-the-fly JavaScript minification

The numbers in #4 are great! If we can get something like that using peast agreed that'll be good enough for most people.

Now that it's not a blocking operation for HTML page requests, and that aggregates can be requested in parallel instead of created strictly in sequence, we've quite a bit of headroom, and those savings are a lot.

This is postponed on #2258313: Add license information to aggregated assets but only from the point of view of GPL licensing, so I think it's worth a viable patch here regardless to see where we end up.

Used Peast per nod_'s suggestion above, we'll need to do a dependency evaluation, but it has everything we need and is easy to use.

I'm attaching a patch that just adds the minification (no line breaks, comments, or optional braces).

I'm also attaching a patch that I used for timing - via hrtime() as well as the output. Processor on my local machine is an 8th generation i5, nothing special. I tested the front page of a 10.1.x standard install logged in a user/1 - this has quite a lot of CSS and JavaScript on it (contextual, toolbar, bigpipe etc.)

Small files like touchevents-test.js take ~1-2ms to process each.

drupal.js and contextual.js take ~18ms each.

ajax.js is the worst at ~97ms.

This is roughly linear with the size of the original files - but note below, we already strip nearly all comments from these files via the js build step

308 touchevents-test.js
6.1K drupal.js
22K  ajax.js

Next: a comparison of aggregate file sizes.
Before:

294K js_J2HoPNZci1FJMi8qdC8t1uBJc1DJta4UfcVkLRWX7zc.js
87K js_J2HoPNZci1FJMi8qdC8t1uBJc1DJta4UfcVkLRWX7zc.js.gz
310 js_qEsgx3cFmLUDeVuvoopSZvsrJF0VpVJ69BQUIa9mOFI.js
235 js_qEsgx3cFmLUDeVuvoopSZvsrJF0VpVJ69BQUIa9mOFI.js.gz

After:

266K js_J2HoPNZci1FJMi8qdC8t1uBJc1DJta4UfcVkLRWX7zc.js
84K js_J2HoPNZci1FJMi8qdC8t1uBJc1DJta4UfcVkLRWX7zc.js.gz
162  js_qEsgx3cFmLUDeVuvoopSZvsrJF0VpVJ69BQUIa9mOFI.js
122  js_qEsgx3cFmLUDeVuvoopSZvsrJF0VpVJ69BQUIa9mOFI.js.gz

On first glance, 294k down to 266k doesn't look that great, but this is deceptive because the aggregate has jQuery and Underscore in it, both of which are pre-minified so not touched by this patch.

jquery.min.js is 88k, underscore is 20k - this is a non-exhaustive list of the pre-minified JavaScript in the aggregate, there's also tabbable, shepherd and others. I think nod_'s numbers above are probably better for actual comparisons.

Also, our own files already have comments stripped by the js build step.

ajax.js is 22k, ajax.es6.js is 59k

Running the same Peast process against ajax.js or ajax.es6.js reduces both to 17k (i.e. the end result should be identical despite very different savings).

Timing Peast compression of ajax.js vs. ajax.es6.js is within 10% of each other, likely because ignoring comments is cheap compared to parsing and formatting the actual code.

ajax.js 60.56
ajax.es6.js 64.89

This means the filesize savings will be more dramatic after #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies without much extra CPU time.

Finally, browser timings from chromium for loading aggregates (and modernizr) - this is on localhost, so the least-friendly test given a 'normal' request would involve network latency.

This shows about 114ms for aggregate generation with no minification, vs about 400ms for aggregate generation with it - i.e. approximately 300ms additional time - more or less matches the raw timings of 100ms + 20ms + 20ms + lots of 1-10ms from the PHP timing.

For the tiny aggregate with hardly anything in it, it took less time to serve which shows the minification is within margin of error when there's not much to minify - the 1 or 2ms that takes is dwarfed by even local network time.

So if we assume the build step is removed, we're looking at hundreds of kb of file size reduction, vs. 200-400ms extra time for asset generation in a relatively bad case of a full admin.

But also we've taken the asset generation out of the page request in #1014086: Stampedes and cold cache performance issues with css/js aggregation - so given CSS + JavaScript aggregates could be 50-150ms each to generate, and we might have five to ten of them, that's 200 to 800ms off the HTML page and time to first byte. That means even if we add some time for minification, we're still overall spending less linear time aggregating on cold starts than Drupal 6 to Drupal 9 was.

And then the aggregates are requested in parallel, so even if the JavaScript aggregate request takes 400ms instead of 115ms, the browser is going to be loading and parsing various CSS aggregates while that's happening. I think this is more than reasonable for the completely cold start case, and every request for an aggregate afterwards will get the smaller file size.

Adding #12 and #15 together.

Per @nod_'s suggestion I thought I'd compare Peast and JsMinify, couldn't find anything called that, but found https://github.com/matthiasmullie/minify and tried that in case that was what he meant and also why not ;)

This is a bit different from Peast:

1. You can minify multiple files at a time if you want as well as telling them where to write to, including a gzip option. Our aggregation architecture currently doesn't allow for this, but it'd be an option for the future. One file at a time is fine too though.
2. It does CSS as well as JS.
3. It does some code shortening for both CSS and JS.

Tried it on ajax.es6.js since that's one of core's larger files, the results are actually a fair bit different:

matthiasmulli/minify
7.99
mck89/peast
104.57

That's more than ten times as fast - presumably because it's mostly running multiple regexps vs. converting to/from AST. This would make a noticeable difference to aggregate generation requests - say we have to minify 20 files and Peast takes an average of 20ms each, that's 400ms vs. more like 40ms for matthiasmulli/minify. Could also mean it's more prone to hard-to-fix bugs: https://github.com/matthiasmullie/minify/issues has a fair number of open reports.

Peast however results in a 200b smaller filesize, not much difference but could add up a few kb across all files in all aggregates.

ls -l /tmp/minify.js 
-rw-r--r-- 1 catch catch 17418 Aug 24 15:27 /tmp/minify.js

ls -l /tmp/peast.js 
-rw-r--r-- 1 catch catch 17204 Aug 24 15:27 /tmp/peast.js

Even though Peast isn't explicitly doing code shortening, it's possible the conversion to and from AST does some implicitly compared to core js code style, so that might be why.

Checked out packagist download stats:

+-------------+-----------------+-------------------+
| Package     | Total Downloads | Monthly Downloads |
+-------------+-----------------+-------------------+
| mck89/peast | 2777381         | 104602            |
+-------------+-----------------+-------------------+
| SUMMARY     | 2777381         | 104602            |
+-------------+-----------------+-------------------+

+-----------------------+-----------------+-------------------+
| Package               | Total Downloads | Monthly Downloads |
+-----------------------+-----------------+-------------------+
| matthiasmullie/minify | 8814552         | 296582            |
+-----------------------+-----------------+-------------------+
| SUMMARY               | 8814552         | 296582            |
+-----------------------+-----------------+-------------------+

Given the download stats depend on things like CI jobs etc., not really much in it - they've both got multiple contributors, dozens of forks, recent stable releases etc.

Uploading the (very basic, just run drush scr ./minify-timing.php) script I used to produce the files and do the timing. Need to install both libraries via composer first.

@nod_ suggested trying JShrink in Slack, here's the results:

https://github.com/tedious/JShrink

tedjvm/jshrink
11.82
matthiasmulli/minify
8.37
mck89/peast
102.94

ls -l /tmp/jshrink.js 
-rw-r--r-- 1 catch catch 17609 Aug 24 18:19 /tmp/jshrink.js

+----------------+-----------------+-------------------+
| Package        | Total Downloads | Monthly Downloads |
+----------------+-----------------+-------------------+
| tedivm/jshrink | 18460829        | 348812            |
+----------------+-----------------+-------------------+
| SUMMARY        | 18460829        | 348812            |
+----------------+-----------------+-------------------+

Still nearly ten times as fast as Peast, slightly higher filesize again, but I think this one might be the right trade-off: explicitly mentions minification on the fly, has a security policy, very few open bug reports etc.

10ms to cut 50kb off a file is probably enough to improve performance even for the initial cache hit that generates the files just reducing that initial transfer.

Switching to JShrink and fixing the test failures.

@Wim Leers looking at the bug reports against matthiasmullie/minify it looks like at least theoretically it might end up introducing errors where Peast wouldn't.

However, the issue queue of JShrink is a fair bit healthier, it has regular releases, seems well supported etc., and it's still ten time faster than Peast, so I don't think there's a good reason to choose Peast instead. The only reason would be if we also wanted to use Peast's AST parsing for some other purpose to only have one dependency, but we're not trying to do that yet.

uhhhhh

Checking core/tests/Drupal/Tests/Core/Asset/js_test_files/latin_9.js.optimized.js

FAILURE core/tests/Drupal/Tests/Core/Asset/js_test_files/latin_9.js.optimized.js does not have a corresponding core/tests/Drupal/Tests/Core/Asset/js_test_files/latin_9.js.optimized.es6.js

Might be blocked on #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies unless we workaround that.

@longwave thanks for finding those two issues against JShrink, if there was the infrequent code changes without those issues that'd be a sign it's just working great, but instead it puts it in the same bucket as matthiasmulli/minify then, so reckon we're back to Peast.

@mfb

Is there any reason not to use JS tooling to build front-end assets? That way, a known-good parser could be used to transform JS, and source maps could be built too for easier debugging. Core could distribute the source JS, minified JS and source maps.

@mfb this was discussed a lot in this 15 year old issue: #119441: Compress JS aggregation where we ended just giving up. Things have moved on a bit since then, but not really that much in the scheme of things except that a lot of the groundwork for both approaches has been done in the meantime.

There are trade-offs in both directions:

1. We could repurpose the build step that's just about to be removed in #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies, to fully minify the foo.js files and keep the foo.es6.js files in place (or rename them to foo.source.js or something). We can't do foo.js/foo.min.js without updating all the library definitions (and breaking library alters etc.).

- minification done by js (very good)
- no performance implications from minification on the fly (good)
- keep the build step (probably fine?)
- no change for js from custom or contrib modules unless they also run the build step and switch to the same file structure. This means more work for contrib and less actual front-end performance impact for real sites unless it's done. (not great)

2. This issue: use JShrink or Peast to minify on the fly.

- minification in PHP (historically bad, but if Peast is still viable then fine)
- performance impact from minification on the fly (manageable even with Peast, but it's definitely there)
- no build step (about to remove it anyway)
- js from custom and contrib modules also gets minified (very good)

The last reason for doing minification on the fly would be if we were to adopt a much more aggressive minification that shortens variable names etc. - since if multiple files are using the same variable name, they could all use the same shortened version, saving even more space. However, the benefits of doing that are very small compared to just removing whitespace and none of the current approaches even begin to try to do that, so I think we can just ignore that for now and assume whatever we do it will be file by file.

So it really comes down to whether contrib/custom js gets minification 'for free' because core's doing it on the fly, vs. using js to minify core's js and hoping contrib follows suit.

The big difference between now and ten+ years ago is that all of the groundwork (apart from licensing which is close) is in place. It's only a few minutes to move the current patch back to Peast so I'll do that to get something working. I think it's probably viable to do minification in the build step (instead of removing the build step altogether), but mostly concerned that we do one of them instead of it taking another 15 years.

Back to Peast. Need to re-do the issue summary update for the dependency evaluation.

@nod_ I think mfb's suggestion is to do minification in the build step, not on-the-fly something like this:

- Instead of shipping with ajax.js and ajax.es6.js, we'd ship with ajax.js (minified), and ajax.source.js (ajax.es6.js renamed).

- library definitions would continue to point to ajax.js, since that's minified, and add {minified: true} to the library definition.

To get minification of contrib or custom code, they'd need to do the same file structure/library definition update.

So this would mean retaining, but repurposing the build step, and the js dev dependencies (+ new ones), but not requiring node.js on production since everything's shipped pre-minified.

It is obviously way more efficient for sites if they literally never have to minify anything, so it's a question of implementing it and maintaining the build step over time vs. the Peast approach.

Yes #35 is great, let's do that!

I've made a start on the dependency evaluation for Peast and tried to reflect the past couple of comments in the issue summary.

@Wim Leers #38 is functionally the same patch as was tested in #9 so the numbers are still valid if we only did this issue.

To be able to redo those timings with the 'new' #35 approach, we'd first need to repurpose the JavaScript build step to minify JavaScript files, and also add minified: true to the respective assets in library definitions. We don't have an issue for that yet, should probably be its own issue separate from this one since the change will be completely different.

Once that's done, we could compare the timings to #9, but it'd also be exactly the same as in HEAD because no minification would be happening and the code added here would be nearly a no-op, there'd just be smaller pre-minified assets to serve.

To test the hybrid approach (pre-minification for core, Peast minification for everything else), we'd need a custom-js-heavy contrib module too. And the asset generation would be whatever it is now + however many miniseconds for Peast to minify the files from that contrib module.

@Wim well the question is really whether this issue is postponed on the pre-minification of core files or not. I've just opened #3308122: Pre-minify core JavaScript so that we can start working on that.

Without that issue, the numbers for core will be the same as #9.

With that issue, the numbers for core will be significantly better than #9, varying on just exactly how many js-heavy contrib modules with unminified JavaScript are in use.

A big motivator behind #1014086: Stampedes and cold cache performance issues with css/js aggregation (apart from the improvements to stability) was moving aggregate generation out of the critical path of generating HTML requests, so that we could then do expensive things like minification. So if we hadn't come up with @longwave's hybrid approach in #35 I probably would still have argued to commit the Peast version of this despite the performance 'regression', because when combined with other changes (and real life front end performance as soon as the aggregates are generated) it would still be a performance improvement overall.

Re-titling so it's easier to tell the difference between this and #3308122: Pre-minify core JavaScript.

Obviously would be great if he's said 'definitely going to be maintained at least the next decade' but also that's a very open and honest answer and he seems happy to accept help.

New patch - needed a re-roll for composer.lock changes, and adding caching of the Peast step.

Doesn't include pre-warming yet.

Yeah I personally think a follow-up would be fine for pre-warming. The caching in the current patch means that aggregates for anon vs. auth vs. front page vs. forms vs. admin pages won't be minifying the same files more than once already.

Fixing the typos (should've been in the last patch but I uploaded the wrong one :/).

#53.1: the param docs are indeed missing, but the rest is #3278431: [May 2026] Use PHP 8 constructor property promotion for existing code! Another example in #1014086: Stampedes and cold cache performance issues with css/js aggregation. Trying to do it for new code, existing code is waiting until post-beta or later. Also @longwave's right we should do basic bc handling for the parameter.

#53.2: hmm given that's in comments and they're being stripped, we probably can! But nod_'s right this is a bit of a behaviour change so maybe in a follow-up so it gets its own CR etc?

#53.3: see #3302755-26: On-the-fly JavaScript minification.

This hopefully does it for the constructor bc. Still adjusting to the combination of property promotion + bc.

protected ?CacheBackendInterface $cache,

I was going to say I'm not a huge fan of multi-line parameter declarations because I always want to add the trailing comma and PHP doesn't allow it. But PHP 8.0 does allow it!

Interdiff is right, patch was wrong :(

There's some implicit whitespace removal with some of the test file changes, but yeah let's add a more explicit one.

Looking at FileCache, first a re-roll.

Should look something like this. There are not that many files to minify and the minified versions should be in the hundreds of kilobytes, so I think we can just use the apcu backend, if that's a concern I think adding a database backend should be in a follow-up.

Isn't that already very quickly full on more complex sites?

Well maybe but asset aggregation tends to be a lot of requests for a few minutes, then very few after that, so if it's happening after a cache clear and items then get expired that seems OK? I don't particularly mind adding a database FileCache backend but it seems out of scope here, would prefer to get this in without any caching at all than blocking it on implementations for unrelated components.

And … "hundreds of kilobytes" really just depends on the amount of JS on a particular site, doesn't it?

Yes, and it'll also be affected by #3308122: Pre-minify core JavaScript which will mean the logic here only happens as a fallback for contrib/custom JS that's not pre-minified.

Test failures are all unrelated, chromedriver is having bad day.

Fixing $cid.

I did some basic timings in #7 which should still be relevant for the current patch.

Small files like touchevents-test.js take ~1-2ms to process each.

drupal.js and contextual.js take ~18ms each.

ajax.js is the worst at ~97ms.

Any file that's 10-100ms the caching will be a net-positive if we get one or more cache hits.

However, the question then is what is the cache hit rate going to be like.

I took a look at the js directory of one production Drupal 9.4 site - this is a fairly complex site although it is not high-traffic.

js aggregates are kept around for 6 weeks with the current logic, so that gives us data since September 7th.

ajax.js was in 276 different aggregates produced since September 7th. This amounts to 27 seconds of minification time if we were minifying it each time.

This site has had a handful of deployments since September 7th, and the files are mostly created during deployments, then not until the next one. So you tend to get 5-20 aggregates per-deployment (anonymous, regular user, admin account, couple of different pages with different JavaScript on them etc.) then a gap in dates until the next tone.

If someone wants to do a comparison on a different site, I just did some grepping for toConsumableArray, this will tell you how many aggregates with that string were created in the past seven days:

find ./ -type f -mtime -7  -print0 | xargs -0 grep -l toConsumableArray | wc -l

On this site it was 44 in the past seven days.

Based on this I think it is reasonable to say that a file like ajax.js will probably get minified 5-10 times almost immediately after a deployment, and then barely touched until the next deployment. This means we'd save 500ms to 1s from caching minification of that file in total across aggregate requests, and if it then gets chucked out of the apcu cache later no-one will notice.

Given all that I don't think it's critical to add caching here at all, it's definitely a nice to have (and especially once we pre-minify core JavaScript since those won't need to be minified or cached at all), but it will slightly help the immediate cold-cache situation you get after deployments in those first few seconds/minutes.

Discussed the caching a bit more with @alexpott. Although it's likely it will reduce the number of times the same file gets minified, we also have other ways to do that:

- pre-minification of core JavaScript (so on-the-fly only runs for contrib and custom JavaScript)
- reducing the variations in aggregates (i.e. by making ordering more consistent)

Also the hit rate is always going to be low - a dozen or so hits at a time.

So... new version with caching removed, if it turns out we really do need it, we could add it in a follow-up.

I think it could make sense to add that test coverage, but only if we weren't also planning to do #3308122: Pre-minify core JavaScript which would make the test coverage redundant again (i.e. that issue renders this code a no-op with only core JavaScript so there'd be no size reduction because nothing would be minified with Peast in core), given that I'm -1 on adding something we'll (hopefully) have to remove again in a few weeks/months.

Just putting back the stray comment how we found it.

@Wim I'm not sure how a test module would be particularly more realistic than adding a bigger source file to the minification unit test and comparing the bytes there. But yes let's discuss in a follow-up.