Skip to content

Commit 415ad8c

Browse files
nicolas-grekasnicolas-grekas
committed
bug #52330 [AssetMapper] Fixing memory bug where we stored way more file content than needed (weaverryan)
This PR was merged into the 6.4 branch. Discussion ---------- [AssetMapper] Fixing memory bug where we stored way more file content than needed | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes (on Symfonycasts, without this, `asset-map:compile` takes >500mb of memory) | New feature? | no | Deprecations? | none | Issues | None | License | MIT Hi! This drastically improves the memory footprint when ALL assets are built, which happens in `asset-map:compile`, `debug:asset-map`, etc. Blackfire is a huge fan! https://blackfire.io/profiles/compare/4eb36732-8805-4c49-b636-d4bf8f9e2b27/graph The MAIN optimization by far is to set `MappedAsset.content` to null if the final, compiled content matches the source file's content. This is the case for the vast-majority of mapped assets, and so the result is MUCH less content stored in memory for no reason. Thanks to `@smnandre` and his earlier PR, which allows for the `null` content on mapped assets. Cheers! Commits ------- 78ec570 [AssetMapper] Fixing memory bug where we stored way more file content than needed
2 parents 60dec0b + 78ec570 commit 415ad8c

File tree

5 files changed

+42
-27
lines changed

5 files changed

+42
-27
lines changed

‎src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php

+1-3
Original file line numberDiff line numberDiff line change
@@ -104,11 +104,9 @@ private function shortenPath(string $path): string
104104

105105
private function createManifestAndWriteFiles(SymfonyStyle $io): array
106106
{
107-
$allAssets = $this->assetMapper->allAssets();
108-
109107
$io->comment(sprintf('Compiling and writing asset files to <info>%s</info>', $this->shortenPath($this->assetsFilesystem->getDestinationPath())));
110108
$manifest = [];
111-
foreach ($allAssets as $asset) {
109+
foreach ($this->assetMapper->allAssets() as $asset) {
112110
if (null !== $asset->content) {
113111
// The original content has been modified by the AssetMapperCompiler
114112
$this->assetsFilesystem->write($asset->publicPath, $asset->content);

‎src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php

+11-15
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ class MappedAssetFactory implements MappedAssetFactoryInterface
2626

2727
private array $assetsCache = [];
2828
private array $assetsBeingCreated = [];
29-
private array $fileContentsCache = [];
3029

3130
public function __construct(
3231
private readonly PublicAssetsPathResolverInterface $assetsPathResolver,
@@ -46,14 +45,15 @@ public function createMappedAsset(string $logicalPath, string $sourcePath): ?Map
4645
$isVendor = $this->isVendor($sourcePath);
4746
$asset = new MappedAsset($logicalPath, $sourcePath, $this->assetsPathResolver->resolvePublicPath($logicalPath), isVendor: $isVendor);
4847

49-
[$digest, $isPredigested] = $this->getDigest($asset);
48+
$content = $this->compileContent($asset);
49+
[$digest, $isPredigested] = $this->getDigest($asset, $content);
5050

5151
$asset = new MappedAsset(
5252
$asset->logicalPath,
5353
$asset->sourcePath,
5454
$asset->publicPathWithoutDigest,
55-
$this->getPublicPath($asset),
56-
$this->compileContent($asset),
55+
$this->getPublicPath($asset, $content),
56+
$content,
5757
$digest,
5858
$isPredigested,
5959
$isVendor,
@@ -75,15 +75,15 @@ public function createMappedAsset(string $logicalPath, string $sourcePath): ?Map
7575
*
7676
* @return array{0: string, 1: bool}
7777
*/
78-
private function getDigest(MappedAsset $asset): array
78+
private function getDigest(MappedAsset $asset, ?string $content): array
7979
{
8080
// check for a pre-digested file
8181
if (preg_match(self::PREDIGESTED_REGEX, $asset->logicalPath, $matches)) {
8282
return [$matches[1], true];
8383
}
8484

8585
// Use the compiled content if any
86-
if (null !== $content = $this->compileContent($asset)) {
86+
if (null !== $content) {
8787
return [hash('xxh128', $content), false];
8888
}
8989

@@ -95,27 +95,23 @@ private function getDigest(MappedAsset $asset): array
9595

9696
private function compileContent(MappedAsset $asset): ?string
9797
{
98-
if (\array_key_exists($asset->logicalPath, $this->fileContentsCache)) {
99-
return $this->fileContentsCache[$asset->logicalPath];
100-
}
101-
10298
if (!is_file($asset->sourcePath)) {
10399
throw new RuntimeException(sprintf('Asset source path "%s" could not be found.', $asset->sourcePath));
104100
}
105101

106102
if (!$this->compiler->supports($asset)) {
107-
return $this->fileContentsCache[$asset->logicalPath] = null;
103+
return null;
108104
}
109105

110106
$content = file_get_contents($asset->sourcePath);
111-
$content = $this->compiler->compile($content, $asset);
107+
$compiled = $this->compiler->compile($content, $asset);
112108

113-
return $this->fileContentsCache[$asset->logicalPath] = $content;
109+
return $compiled !== $content ? $compiled : null;
114110
}
115111

116-
private function getPublicPath(MappedAsset $asset): ?string
112+
private function getPublicPath(MappedAsset $asset, ?string $content): ?string
117113
{
118-
[$digest, $isPredigested] = $this->getDigest($asset);
114+
[$digest, $isPredigested] = $this->getDigest($asset, $content);
119115

120116
if ($isPredigested) {
121117
return $this->assetsPathResolver->resolvePublicPath($asset->logicalPath);

‎src/Symfony/Component/AssetMapper/MappedAsset.php

+3-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ final class MappedAsset
2626
public readonly string $publicExtension;
2727

2828
/**
29-
* The final content of this asset, if different from the source.
29+
* The final content of this asset if different from the sourcePath.
30+
*
31+
* If null, the content should be read from the sourcePath.
3032
*/
3133
public readonly ?string $content;
3234

‎src/Symfony/Component/AssetMapper/Tests/AssetMapperDevServerSubscriberFunctionalTest.php

+5-2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
1515
use Symfony\Component\AssetMapper\Tests\Fixtures\AssetMapperTestAppKernel;
16+
use Symfony\Component\HttpFoundation\BinaryFileResponse;
1617

1718
class AssetMapperDevServerSubscriberFunctionalTest extends WebTestCase
1819
{
@@ -23,11 +24,12 @@ public function testGettingAssetWorks()
2324
$client->request('GET', '/assets/file1-b3445cb7a86a0795a7af7f2004498aef.css');
2425
$response = $client->getResponse();
2526
$this->assertSame(200, $response->getStatusCode());
27+
$this->assertInstanceOf(BinaryFileResponse::class, $response);
2628
$this->assertSame(<<<EOF
2729
/* file1.css */
2830
body {}
2931
30-
EOF, $response->getContent());
32+
EOF, $response->getFile()->getContent());
3133
$this->assertSame('"b3445cb7a86a0795a7af7f2004498aef"', $response->headers->get('ETag'));
3234
$this->assertSame('immutable, max-age=604800, public', $response->headers->get('Cache-Control'));
3335
$this->assertTrue($response->headers->has('X-Assets-Dev'));
@@ -59,11 +61,12 @@ public function testPreDigestedAssetIsReturned()
5961
$client->request('GET', '/assets/already-abcdefVWXYZ0123456789.digested.css');
6062
$response = $client->getResponse();
6163
$this->assertSame(200, $response->getStatusCode());
64+
$this->assertInstanceOf(BinaryFileResponse::class, $response);
6265
$this->assertSame(<<<EOF
6366
/* already-abcdefVWXYZ0123456789.digested.css */
6467
body {}
6568
66-
EOF, $response->getContent());
69+
EOF, $response->getFile()->getContent());
6770
}
6871

6972
protected static function getKernelClass(): string

‎src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php

+22-6
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,22 @@ public function testCreateMappedAssetRespectsPreDigestedPaths()
4848
$this->assertSame('/final-assets/already-abcdefVWXYZ0123456789.digested.css', $asset->publicPathWithoutDigest);
4949
}
5050

51-
public function testCreateMappedAssetWithContentBasic()
51+
public function testCreateMappedAssetWithContentThatChanged()
5252
{
53-
$assetMapper = $this->createFactory();
54-
$expected = <<<EOF
55-
/* file1.css */
56-
body {}
53+
$file1Compiler = new class() implements AssetCompilerInterface {
54+
public function supports(MappedAsset $asset): bool
55+
{
56+
return true;
57+
}
5758

58-
EOF;
59+
public function compile(string $content, MappedAsset $asset, AssetMapperInterface $assetMapper): string
60+
{
61+
return 'totally changed';
62+
}
63+
};
64+
65+
$assetMapper = $this->createFactory($file1Compiler);
66+
$expected = 'totally changed';
5967

6068
$asset = $assetMapper->createMappedAsset('file1.css', __DIR__.'/../Fixtures/dir1/file1.css');
6169
$this->assertSame($expected, $asset->content);
@@ -65,6 +73,14 @@ public function testCreateMappedAssetWithContentBasic()
6573
$this->assertSame($expected, $asset->content);
6674
}
6775

76+
public function testCreateMappedAssetWithContentThatDoesNotChange()
77+
{
78+
$assetMapper = $this->createFactory();
79+
$asset = $assetMapper->createMappedAsset('file1.css', __DIR__.'/../Fixtures/dir1/file1.css');
80+
// null content because the final content matches the file source
81+
$this->assertNull($asset->content);
82+
}
83+
6884
public function testCreateMappedAssetWithContentErrorsOnCircularReferences()
6985
{
7086
$factory = $this->createFactory();

0 commit comments

Comments
 (0)