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.

Files: 
CommentFileSizeAuthor
#10 drupal8.base-system.2114165-10.patch557.83 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2114165-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 drupal8.base-system.2114165-6.patch1.45 MBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 59,364 pass(es).
[ View ]
#2 drupal8.base-system.2114165-1.patch1.46 MBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2114165-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs work

patch lost (

Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new1.46 MB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2114165-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ehh, 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.

Status:Needs review» Reviewed & tested by the community

diff --git a/composer.json b/composer.json
index 436dae9..3d33c6d 100644
--- a/composer.json
+++ b/composer.json
@@ -4,6 +4,7 @@
   "type": "drupal-core",
   "license": "GPL-2.0+",
   "require": {
+    "mrclay/minify": "2.1.*",
     "sdboyer/gliph": "0.1.*",
     "symfony/class-loader": "2.3.*",
     "symfony/dependency-injection": "2.3.*",

#2: drupal8.base-system.2114165-1.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.base-system.2114165-1.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.45 MB
PASSED: [[SimpleTest]]: [MySQL] 59,364 pass(es).
[ View ]

reroll, should work. restoring RTBC.

Assigned:Unassigned» Dries
Issue summary:View changes

Adds a new library so moving to Dries.

Status:Reviewed & tested by the community» Needs work

actually...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.

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new557.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.base-system.2114165-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok, 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 named CSSmin. 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.

data for edit.theme.css
Array
(
    [time] => Array
        (
            [minify] => 0.016247034072876
            [cssmin] => 0.022017955780029
            [drupal] => 0.00045490264892578
        )
    [strlen] => Array
        (
            [minify] => 4452
            [cssmin] => 6536
            [drupal] => 4533
        )
)
data for system.theme.css
Array
(
    [time] => Array
        (
            [minify] => 0.028863906860352
            [cssmin] => 0.069223165512085
            [drupal] => 0.0018179416656494
        )
    [strlen] => Array
        (
            [minify] => 8683
            [cssmin] => 10522
            [drupal] => 9299
        )
)
data for toolbar.icons.css
Array
(
    [time] => Array
        (
            [minify] => 0.051509857177734
            [cssmin] => 0.031339168548584
            [drupal] => 0.003756046295166
        )
    [strlen] => Array
        (
            [minify] => 10949
            [cssmin] => 11278
            [drupal] => 14121
        )
)
data for views_ui.admin.theme.css
Array
(
    [time] => Array
        (
            [minify] => 0.040516138076782
            [cssmin] => 0.061883926391602
            [drupal] => 0.0014970302581787
        )
    [strlen] => Array
        (
            [minify] => 14609
            [cssmin] => 15089
            [drupal] => 15222
        )
)

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 :)

Status:Needs review» Needs work

The last submitted patch, 10: drupal8.base-system.2114165-10.patch, failed testing.

Status:Needs work» Needs review

omgwtfbbq, OOM error on a test?

Sam, 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.)

@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.

Given 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.

"Minify produces a great result, but has memory issues while doing so."

it'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.

the strlen is the length of the css string after minification had been run

is 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.

Status:Needs review» Needs work

The last submitted patch, 10: drupal8.base-system.2114165-10.patch, failed testing.

Title:Update Assetic in composer, and add its actual dependenciesAdd mrclay/minify vendor library to core

Updating 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?

Issue summary:View changes

I'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.

related - turns out our current minification logic actually breaks CSS. awesome. #936316: CSS aggregation strips some essential whitespace within strings

Issue tags:+Needs reroll