Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minify sometimes takes 30x longer #280

Open
davidscotson opened this issue Mar 29, 2019 · 7 comments
Open

Minify sometimes takes 30x longer #280

davidscotson opened this issue Mar 29, 2019 · 7 comments

Comments

@davidscotson
Copy link

I'm currently investigating an issue, that seems broadly similar to #222, but regarding CSS and seeing the big variance in times even on the same server. But similarly it seems to only be happening on one server for us.

Something that usually takes 7-10s sometimes takes 2-3 minutes (or it would if we didn't have a 30s timeout). The result gets cached after it succeeds, and a few refreshes usually fixes it so we had assumed it was just taking maybe 3 times as long sometimes when the server was busy. After investigating it and raising the timeout we were suprised that something that can complete usually in about 7 seconds can take so much longer sometimes.

This is using 1.3.37 and like #222 all the time seems to be in the replace function, about 2/3rds of time and memory on substr and the rest of on replace_pattern.

I'm trying to track down what changes between the fast and slow runs. I will get the input and output for a short and long run and see if I can spot any differences between them and then try to see what the code is doing differently but thought I'd open a bug in case the answer is obvious to someone with more familiarity.

@davidscotson
Copy link
Author

Currently, just the size of the input seems to be what is triggering this. Over 1MB seems to be the critical size on the machine we see this on.

@davidscotson
Copy link
Author

A small test script that can be run from the Moodle/Totara root directory on a test CSS file to time how long it takes on a server:

<?php
require_once 'lib/minify/matthiasmullie-minify/src/Minify.php';
require_once 'lib/minify/matthiasmullie-minify/src/CSS.php';
require_once 'lib/minify/matthiasmullie-pathconverter/src/Converter.php';
use MatthiasMullie\Minify;
raise_memory_limit(MEMORY_UNLIMITED);
$minifier = new Minify\CSS('test.css');
$minifier->minify('core.min.css');

@matthiasmullie
Copy link
Owner

That's odd. I'd have to investigate further, but I have a feeling that, if larger files need to be minified, things get too big to stuff in memory and the machine starts swapping, which would explain why it suddenly takes orders of magnitude more to process.

@timhunt
Copy link
Contributor

timhunt commented Mar 17, 2020

@davidscotson is a name a recognise. I was seeing crazy slow minification performace in our Moodle, so I put in some debugging to work out which regexp was being slow, and I was surprised to find that it as all this regexp:

'/\n?/*(!|.?@license|.?@preserve).*?*/\n?/s'

https://github.com/matthiasmullie/minify/blob/master/src/CSS.php#L639

That was only used in 2 calls to preg_match (out of 1170), but those 2 calls took 15 seconds(!) on our (admittedly very long) input.

I will now see if I can improve that.

@timhunt
Copy link
Contributor

timhunt commented Mar 17, 2020

OK, got it!

The problem was that the .* before the @Licence could match anything, including /. Therefore, it was (slowly) consuming vast chunks of the input. The following runs in a fraction of a second, rather than 15 seconds, and results in all of the CSS being minimised. (Previously, everything between the first / and the first @Licence was being left un-squished.

        $incommentchar = '(?:(?!\*\/).)';
        $this->registerPattern('/\n?\/\*(?:!|' . $incommentchar . '*?@(?:license|preserve)).*?\*\/\n?/s', $callback);

The only down-side is that this makes the regexp horribly unreadable.

Shall I see if I can make a pull request?

@timhunt
Copy link
Contributor

timhunt commented Mar 17, 2020

I just made a separate issue #317 for what I found.

@davidscotson
Copy link
Author

Thanks @timhunt, I'll give that a try to see if it helps my test cases for this issue too.

The other ticket I linked in my first comment here, #222 was JS minification rather than CSS, and it looks like the same regex is used for JS as well so it might be responsible for both issues.

davidscotson pushed a commit to davidscotson/minify that referenced this issue Mar 21, 2020
davidscotson pushed a commit to davidscotson/minify that referenced this issue Mar 22, 2020
davidscotson pushed a commit to davidscotson/minify that referenced this issue Mar 22, 2020
uses Tim Hunt's regex fix for matthiasmullie#333, but moves it so it's done
in a different place to split on, rather than remove, special comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants