We've got a slightly old version of Assetic in composer, and it should be updated.
More importantly, Assetic has a ton of dependencies without which we cannot use most of its filters (e.g., css minification, js minification, css/js preprocessing, etc.). I believe because of how Symfony is expected to work, these dependencies are listed in its require-dev
block, not require
, which means we haven't already pulled them in.
This patch adds some of those dependencies - specifically, one that should let us compress css and js entirely in PHP. We should do this, then remove them later if we decide we don't want them, in order to simplify work over in #1762204: Introduce Assetic compatibility layer for core's internal handling of assets.
Comment | File | Size | Author |
---|---|---|---|
#10 | drupal8.base-system.2114165-10.patch | 557.83 KB | sdboyer |
Comments
Comment #1
podarokpatch lost (
Comment #2
sdboyer CreditAttribution: sdboyer commentedehh, changed my mind, i didn't pull in the preprocessors. really just meant adding one more vendor, at least for now - mrclay/minify. that has basic minifiers for both CSS and JS.
yes, it's big.
it'd be lovely if we could pull this in without too much todo in the spirit of continuing support for #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. this really is nothing more than an Assetic dependency.
Comment #3
RobLoachComment #4
RobLoach#2: drupal8.base-system.2114165-1.patch queued for re-testing.
Comment #6
sdboyer CreditAttribution: sdboyer commentedreroll, should work. restoring RTBC.
Comment #7
catchAdds a new library so moving to Dries.
Comment #8
sdboyer CreditAttribution: sdboyer commentedComment #9
sdboyer CreditAttribution: sdboyer commentedactually...sorry, hold off on this. i need to experiment more with it and #1762204: Introduce Assetic compatibility layer for core's internal handling of assets locally to make sure it's all set up right.
Comment #10
sdboyer CreditAttribution: sdboyer commentedok, i've experimented more.
it'd be lovely if they could size down the portion of the minify library that was actually offered up by composer - yknow, some decomposition, etc. - or at least offer some less-shitty stuff that goes into the
autoload.classmap.php
. the library not being even a little PSR-0, it adds several dozen autoload points. it's not the end of the world, of course, but this patch puts the cost of holding that classmap alone at about 100k of mem (on 64bit).regardless, minify is *highly* aggressive. it's a pure PHP port of the YUI compressor. it's also quite a lot slower than our current logic. now, that doesn't mean we shouldn't use it - minification is something that should, of course, be cached, so it may be worth it. but it's a little scary to think that big sites might see a noticeable spike after a cache clear. it's only CPU intensive, so a thundering herd problem there is less of an issue, but...
to help reduce the size of this patch, i've manually removed minify's tests directory, which is almost three quarters of the total lib size. (a good reminder for myself, actually - gonna go make that an automatic thing in gliph now)
anyway, so - i then tried to add cssmin. there are size concerns there, as well. the library itself is only 160k...but the version of the library i grabbed (by working from assetic's own composer.json) is also ONE FILE with 5k freakin lines, and bloats
autoload.classmap.php
even worse than minify does (mem use purely from holding the declared classmap array file jumps to ~250k). now, this is based on the compacted file version that assetic specifies there - we don't *necessarily* need to use that. but getting autoloading working? ugh.regardless, there's another, TRULY delightful problem. cssmin's entry point class is named
CssMin
; minify's is namedCSSmin
. or, as PHP sees it, there are two classes, both named the same thing. which means it's impossible to use both at the same time without hacking their code. ...which i did, so that we could at least get some numbers for comparison purposes.i set up some rudimentary-but-illustrative tests to benchmark our current filter (identified below as 'drupal') , the cssmin filter, and the minify filter against each other on the same files. the cssmin filter is configurable to be able to do more or less, so in these runs i configured it to do all of its things.
note also that cssmin also uses the /e modifier on
preg_replace()
, which is deprecated (and throws an error) on PHP5.5. i had to drop back down to 5.4 in order to run this.the times are the microtime delta for running the each minifier fully. the strlen is the length of the css string after minification had been run. i selected these particular files because they're some of the largest ones currently in core.
yes, xdebug & xhprof were disabled to reduce bias against large numbers of function calls. the difference was quite noticeable - here, minify and cssmin are roughly equivalent, but with xdebug on, cssmin is typically 5x slower than minify.
so, it looks to me like cssmin is a fairly clear loser here. it is usually slower than minify, and it is always worse at actual minification. our filter actually stacks up pretty well, considering how relatively blazing fast it is (15-30x faster than minify). plus, cssmin has been floating around dead on google code for 2 years (whereas minify is active). and, it uses a singleton pattern with global state registration, including its own failboat autoloader. thumbs down. minify does try to tweak php.ini settings in a way that's a little annoying, but it can be managed.
my take is this: there are some maybe-reasonable concerns about whether or not we actually use something this much slower than what we have now, even if the output is supposed to be cached, since the benefit is often not even a 10% reduction in css string length. however, we don't have to make those decisions right now, and with the reduced size of this patch vs. the previous one, it seems worth putting it in for now so that we can run more tests as we go.
leaving on needs review in hopes that folks will help think through this. if my conclusion seems reasonable, though, please don't hesitate to RTBC :)
Comment #12
sdboyer CreditAttribution: sdboyer commented10: drupal8.base-system.2114165-10.patch queued for re-testing.
Comment #13
sdboyer CreditAttribution: sdboyer commentedomgwtfbbq, OOM error on a test?
Comment #14
Crell CreditAttribution: Crell commentedSam, can you clarify the benefit of using Minify over just wrapping our existing code into classes that match Assetic's interfaces? (I'm assuming the logic would be something like that, having not looked at the code...)
Pulling in a library that does its own autoload black magic rather than PSR-anything rubs me the wrong way; that was one of the two key fails of Imagine to replace our image handling. (Although it looks like it's since gone PSR-0/PHP 5.3. Good for them.)
Comment #15
sdboyer CreditAttribution: sdboyer commented@Crell - minify simply does a better job. that's what the
strlen
data in #10 is about. the smaller the string, the more compactly the same CSS has been expressed, and the less data has to go over the wire.we could, and probably should, wrap our existing logic into classes that implement Assetic's
FilterInterface
. we need not have an either/or choice here - that's a very nice thing about how Assetic makes this work.and i agree, a library doing its own crappy autoload is an independently sufficient basis to reject it. i'm just glad that the offending library is the one that's also inferior on other fronts.
Comment #16
Crell CreditAttribution: Crell commentedGiven where we are in the release cycle, I'm inclined to go for "stable interfaces, easy to write code" first over "smaller result but potentially problematic performance trade-offs" code, at least for now. If I follow the above correctly (and if not, let me know), it comes down to "our existing routine is good for runtime performance but produces a suboptimally compressed result" vs. "Minify produces a great result, but has memory issues while doing so."
Since switching from one to the other is not an API change as long as it's all behind Assetic interfaces, I'd suggest taking the first route for now and punting on Minifier. The win we're after is minification/aggregation that is compatible with isolated block rendering; smaller resulting CSS files are gravy compared to that.
Comment #17
sdboyer CreditAttribution: sdboyer commentedit's CPU cycles, not memory, but otherwise, yes, that's basically accurate.
it's actually more work over in #1762204: Introduce Assetic compatibility layer for core's internal handling of assets to refactor Drupal's current logic and put it behind symfony's
FilterInterface
than it is to just totally switch over to using minify. but, yes, less complicated in other ways. i suppose, though, the thing to do is just refactor what we have now behind the new interface, use that, and let this issue proceed as it may.that said - minification/aggregation being compatible with isolated block rendering is not relevant here. that's what the main assets patch moves us towards (though, to be clear, it does not independently achieve that - still need a well-structured page model, blah blah blah). the only question we should be asking here is whether or not we want to accept longer processing to regenerate optimized assets in exchange for decreased wire traffic.
well, not quite only - we could also ask it as, "do we want to provide the smaller assets as an easy option to enable, even if it's not on by default?" in that case, we should still pull this in and make it an easy toggle.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedis that the raw length, or after gz compression? seems like only the latter is truly important, and it's not necessarily the case the % diff between the different algorithms will be the same after compression.
Comment #19
effulgentsia CreditAttribution: effulgentsia commented10: drupal8.base-system.2114165-10.patch queued for re-testing.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedUpdating to a later version of Assetic doesn't need approval from Dries, so if you want to just do that, open a new issue for it. Retitling this one to clarify what's being asked of Dries.
Not my call, but I must say though that so far, #17 doesn't seem to me like a compelling reason to do this; do you want to try presenting a more compelling argument?
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedComment #23
Dries CreditAttribution: Dries commentedI'm personally concerned that this approach doesn't buy us much, so I'd prefer to leave the Drupal-specific logic in for now.
Here is why:
So let's reroll the patch without Minify for now.
Comment #24
sdboyer CreditAttribution: sdboyer commentedrelated - turns out our current minification logic actually breaks CSS. awesome. #936316: CSS aggregation strips some essential whitespace within strings
Comment #25
martin107 CreditAttribution: martin107 commentedComment #26
idebr CreditAttribution: idebr commentedComment #27
zpawn CreditAttribution: zpawn commentedComment #28
Wim LeersIssue summary update definitely still needed, but at least clarifying the title for now and moving to a more appropriate component.
Comment #41
catchThe library situation will have changed by now, but this is still potentially useful.
Properly postponing on #1014086: Stampedes and cold cache performance issues with css/js aggregation since there's no way we can do it while css/js aggregation is a page-request-blocking operation.
Comment #43
catchComment #44
catchActually, this started as an issue to update Assetic, which was in core at the time, but isn't now, and it doesn't really discuss the general issue of switching to a third party CSS minification library at all, so i've gone ahead and opened a new issue with less confusing history, and marking this as duplicate. #3302612: Consider adopting a third party CSS minification library