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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

podarok’s picture

Status: Active » Needs work

patch lost (

sdboyer’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.46 MB

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.

RobLoach’s picture

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.*",
RobLoach’s picture

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

sdboyer’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.45 MB

reroll, should work. restoring RTBC.

catch’s picture

Assigned: Unassigned » Dries
Issue summary: View changes

Adds a new library so moving to Dries.

sdboyer’s picture

sdboyer’s picture

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.

sdboyer’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
557.83 KB

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.

sdboyer’s picture

Status: Needs work » Needs review
sdboyer’s picture

omgwtfbbq, OOM error on a test?

Crell’s picture

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

sdboyer’s picture

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

Crell’s picture

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.

sdboyer’s picture

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

effulgentsia’s picture

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.

effulgentsia’s picture

Status: Needs review » Needs work

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

effulgentsia’s picture

Title: Update Assetic in composer, and add its actual dependencies » Add 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?

effulgentsia’s picture

Issue summary: View changes
Dries’s picture

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.

sdboyer’s picture

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

martin107’s picture

Issue tags: +Needs reroll
idebr’s picture

Assigned: Dries » Unassigned
zpawn’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs issue summary update
Wim Leers’s picture

Title: Add mrclay/minify vendor library to core » Consider adopting 3rd party CSS minification library
Component: base system » asset library system
Issue tags: +front-end performance

Issue summary update definitely still needed, but at least clarifying the title for now and moving to a more appropriate component.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Title: Consider adopting 3rd party CSS minification library » [PP-1] Consider adopting 3rd party CSS minification library
Status: Needs work » Postponed

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

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Title: [PP-1] Consider adopting 3rd party CSS minification library » Consider adopting 3rd party CSS minification library
Status: Postponed » Active
catch’s picture

Status: Active » Closed (duplicate)
Related issues: +#3302612: Consider adopting a third party CSS minification library

Actually, 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