Repository navigation
Allowed memory limit exceeded when unserializing cache. #104
Description
Activity
@JohntheFish Are you able to share a test case to reproduce this?
For example:
$lessFile = __DIR__ . '/myinput.less'; // no cache $parser = new Less_Parser(); $parser->parseFile( $lessFile ); $parser->getCss(); // cache clear // $ rm -rf /tmp/less_php_cache/
// cache miss $options = [ 'cache_dir' => '/tmp/less_php_cache/' ]; $files = [ $lessFile => '' ]; Less_Cache::Get( $lessFiles, $options )
// cache hit Less_Cache::Get( $lessFiles, $options )
If you run this from an ad-hoc php script, does it run out of memory only in the cache hit case, or also in one or both of the no-cache or cache-miss cases?
It looks like in your case, you're running out of memory by possilby only a small margin (slightly more than the limit of 128M). In that case, it would be useful to determine the following:
- How much more does it need to succeed?
- How small can you make the limit before the no-cache script fails as well?
You can run the script like
php -d memory_limit=256M myscript.phpand increase/decrease accordingly.This would help rule out whether the majority of the memory consumption (and possible leak) is in how the tree is represented in general, or whether there is a notable cost that is specific to caching. I suspect the caching is only making a small difference, and that perhaps your input exposes a general problem in how the tree is represented.
If so, I would need a copy of your input file or a simplified version of it. I suspect there is a specific kind of syntax in one of your files that is represented in a recursive or otherwise very inefficient manner, however, that is difficult to find without an example.
For example, if your input file exceeds, let's say, 64M of memory even without caching, then caching is likely not the real issue, but something in how Less.php represents your input file. In that case, I would suggest iteratively removing half your input file and narrow down bit by bit what kind of Less syntax is triggering the leak. If you're comfortable sharing the entire input file/directory, I could help with that. GitHub allows attaching ZIP files as well.
I can confirm it only hits the memory limit in the cache hit case.
- No cache always works
- A cache miss always works
- A cache hit fails on some files. The file content is successfully read, but the memory limit is hit by unserialize.
Upping the memory limit resolves the problem. (I doubled it to 256M).
For some more diagnostics, I set it down again to 128M.
Working through the source files individually (or minimal subsets of them where there is interdependence), no individual file (or minimal subset) results in the issue. They all cache and cache reading is always successful.
Going back to the overall compilation, I added some logging to identify the last file read from cache before an unserialize broke. This identified a respective source file, but commenting that source out of the overall resulted in it hitting a memory limit on unserialize of a cache file a few files later. The unserialize tended to break on unserializing cache files originating from larger source files rather than any particularly complex rules. However, if the cause of the problem is cumulative consumption of memory by successive unserialize, that would be expected.
Looking for "Unserialize" issues on php github. This issue looks promising: php/php-src#10126
Anyway, thanks for prompting me about memory limits. Increasing to 256M gets round the problem.
A quick experiment to dodge memory limits. The magic fudge factor of 0.5 worked for my current project (0.6 failed). The magic 0.5 is obviously application specific - increase the size of a source file and the magic 0.5 could be invalidated.
With that in mind this code is not a workable solution, also because work has already been done saving a cache file that is not now used.
case 'serialize': if (preg_match('/^(\d+)M$/', ini_get('memory_limit'), $matches)) { if (memory_get_usage() < (0.5 * 1024 * 1024 * $matches[1])) { $cache = unserialize(file_get_contents($cache_file)); } } if ($cache ?? null) { touch($cache_file); $this->UnsetInput(); return $cache; } break;I won't be leaving the above in the code, the simple increase of memory to 256M is less messy (though again I expect application specific)
@JohntheFish Thanks, that makes sense. The 4X increase in memory, as stated at php/php-src#10126, would indeed get you over the limit much more quickly.
I suspect there might still be something we can do to reduce memory usage. For example, if you take any leaf input file and concatenate copies of the same input file until you reach roughly the same SLoC as your actual input file, I suspect it would not reach the limit. That is to say, some input is more "expensive" than others, and probably a certain combination of operators or mixins in your source code, perhaps some kind of seemingly-recursive Less logic, might be leading to a disproportionate amount of memory being consumed, compared to other input code with the same number of lines/tokens.
Having said that, let me share what we do for Wikipedia in MediaWiki production. We don't use the cache feature of less.php, in part due to a policy against storing serialized PHP, but also in part for performance. We find we get way better cache-hit performance by storing the resulting CSS code rather than the intermediary Less Tree. This requires a few more lines of code on the caller side, but is something we've been doing since before we adopted the current less.php library.
You can find our code at https://github.com/wikimedia/mediawiki/blob/1.41.0/includes/ResourceLoader/FileModule.php#L1092, but it basically boils down to:
- After a cache miss, store the result of
$parser->getCSS(), along with the result of$parser->AllParsedFiles(), and the result ofmd5( implode( '', array_map($files, 'md5_file') ) ). - Cache this in-memory with
apcu_storeunder a key like'lessphp-css', md5_file($lessFilePath), $lessFilePath, $vars, $importDirs. - On cache-hit, access
$data['files']and do the same md5_file mapping for the current point in time, this tells you whether or not any indirectly imported files have changed. If the combined hash is still the same, then return$data['css']and consider it a real cache hit. Otherwise, treat as cache miss.
If you read the actual MediaWIki code, you'll find it does a few more things. We actually use
hash('md4')instead ofmd5(). In addition we avoid callingmd5_fileand instead try to maximise use of the operating system's fstat cache by caching the result ofhash('md4', file_get_contents()in APCU under a key based onfilepath:mtimefrom filemtime. That way in the common case all we do is read a key from apcu_fetch, and then to validate the hashes the "map" operation only calls filemtime a bunch of times (cheap), with which we then fetch each cached file hash from apcu. We don't use the mtime as the final validation itself we find those aren't reliable over time (i.e. Git doesn't track it, and may not be deterministic).- After a cache miss, store the result of
@JohntheFish I've looked at this a bit more and I see a good path forward I think.
Basically, there are two separate caches at play:
- The fast whole-output cache (in Less_Cache, which stores
.cssand.listfiles). - The incremental cache (in Less_Parser, which stores
.lesscachefiles) which speed up re-parses of unchanged files during a cache miss where at least one file did change.
I believe that for the vast majority of people using this library, the incremental cache is at best a micro-optimization and at worst a waste of memory and (much more) disk space. It also seems very likely to confuse people because it isn't described anywhere as an optimization for cache misses. I could see someone thinking that
Less_Cache::Get()is comparable tonew Less_Parser(['cache_dir' => ..]), parseFile, getCss, and that the latter will somehow use the cache directory in the same way.It is inherent to the Less language that later imports may change variables or extend mixins used in earlier files, and that imports may reference variables defined by earlier imports. Thus one can't actually cache the CSS output of an import. All inputs needs to be re-compiled together to produce the correct CSS output. All the incremental cache does is allows the
parseFile()method to skip parsing for unchanged files (i.e. interpreting Less syntax into an object structure). ThegetCss()method will still traverse and compile the representation of all files.Example: https://gist.github.com/Krinkle/02e3dacdc8e32cc04efdc464593aa262
$ php --version PHP 8.2.27 $ php example.php Less_Cache: 0.20 ms, 16.81 MiB Less_Parser-nocache: 467.70 ms, 16.80 MiB Less_Parser-cache: 351.30 ms, 49.83 MiB $ php example.php Less_Cache: 0.20 ms, 1.42 MiB Less_Parser-nocache: 461.10 ms, 16.80 MiB Less_Parser-cache: 349.10 ms, 49.83 MiBNote that the cache hit several times more memory (we know this already of course, per your report), but... it is also saves relatively little time compared to Less_Cache. I can even imagine it being slower to cache than to not cache, with a faster PHP engine / CPU, and slower disk reads. After all, the bulk of the time isn't spent in parsing files.
To start with, I propose to add an
cache_incrementaloption in Less_Parser that you can turn off by passing it as part of$optionsinLess_Cache::Get(). That way, you only use the CSS output cache, and don't engage the incremental cache for marginal gains on cache misses.- The fast whole-output cache (in Less_Cache, which stores
@JohntheFish This is now released as part of Less.php v5.3.0.
Details at https://github.com/wikimedia/less.php/blob/v5.3.0/API.md#incremental-caching
Let me know if this resolves the issue for you!
php 8.2.14, fpm-fcgi, Apache/2.4.58 (Fedora Linux) OpenSSL/3.1.1
I am getting the error:
wikimedia/less.php/lib/Less/Parser.php:612 Allowed memory size of 134217728 bytes exhausted (tried to allocate 20480 bytes)
The line in question is:
less.php/lib/Less/Parser.php
Line 612 in b01ecdd
For some reason it doesn't like unserializing some rules cached by
less.php/lib/Less/Parser.php
Line 641 in b01ecdd
I have manually cleared out the cache files and tried again, resulting in the same issue. I have also updated to current (today) less.php code from GitHub.
I have not been able to track down the specific cached rules that result in the issue. I have a gut feeling it could be a cumulative memory leak rather than any specific rule, though no hard evidence to back that up.
For now, I have disabled reading the less parser cache (by hacking line 607 to
0 && file_exists) and everything works, albeit without the benefits of the cache.The same .less source compiled under php8.1. But now results in the above under php8.2 (though there are some other environment differences, so php version may be a completely spurious clue).