Skip to content

[AssetMapper] Optimize memory usage (load only "compilable" assets) #52156

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

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

smnandre
Copy link
Member

Q A
Branch? 6.4
Bug fix? no
New feature? no
Deprecations? no
License MIT

A bit late for 6.4... but this PR will have a sensible impact on performances.

Currently if the assets directory contains 500 images, they are all loaded in memory during the compile command.
Their contents are also serialized/unserialized and stored in cache.

This PR allow to load/store in memory/cache only the assets supported by the compilers (JS/CSS files today)

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic - I can't believe how small the diff was to fix this!

@smnandre
Copy link
Member Author

Should i do something special for the Psalm alert ? The code is safe and "hash_file" will return a string, but Psalm has no way to know the file existence has just been tested.... and i don't want to check it twice in the code.

@nicolas-grekas
Copy link
Member

Nope, you can ignore psalm, we didn't surrender to robots (yet) ;)

@smnandre
Copy link
Member Author

So all is ok for me :)

@fabpot fabpot force-pushed the feat/asset-mapper-performances branch from 2fe6f9a to 9f63c81 Compare October 20, 2023 06:02
@fabpot
Copy link
Member

fabpot commented Oct 20, 2023

Thank you @smnandre.

@fabpot fabpot merged commit 18df11d into symfony:6.4 Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants