8000 [AssetMapper] Avoid loading potentially ALL assets in dev server by weaverryan · Pull Request #50394 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Avoid loading potentially ALL assets in dev server #50394

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

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2023
Merged

[AssetMapper] Avoid loading potentially ALL assets in dev server #50394

merged 1 commit into from
May 23, 2023

Conversation

weaverryan
Copy link
Member
Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets None
License MIT
Doc PR Still TODO

Hi!

One other rough edge found when using on a real project. The dev server needs to quickly go from "public path" -> "logical path", so it can then look up the MappedAsset. Previously, for every served asset, it would loop over ALL assets until it found a match. We have a CachedMappedAssetFactory, which makes this happen quickly, but it still loads many assets into memory, including their contents. I was seeing a 13mb jump in the memory on those requests in a modest-sized app. If someone was serving a lot of images, it could get huge. No reason to do that work multiple times.

Btw, none of this happens on production - caching is here just for dev experience.

Thanks!

@carsonbot carsonbot added this to the 6.3 milestone May 23, 2023
@weaverryan weaverryan force-pushed the asset-mapper-dev-public-path-cache branch from 4691fc9 to 2f76e18 Compare May 23, 2023 01:16
chalasr
chalasr approved these changes May 23, 2023
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍

@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas removed the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label May 23, 2023
@weaverryan weaverryan force-pushed the asset-mapper-dev-public-path-cache branch from 2f76e18 to a89b732 Compare May 23, 2023 14:47
@weaverryan
Copy link
Member Author

Just made AssetMapperInterface::allAssets() iterable - a safer bet and easy win.

@nicolas-grekas nicolas-grekas changed the base branch from 6.4 to 6.3 May 23, 2023 16:47
@nicolas-grekas
Copy link
Member

Tests fail :)

@weaverryan weaverryan force-pushed the asset-mapper-dev-public-path-cache branch from f973828 to a340568 Compare May 23, 2023 17:31
@weaverryan
Copy link
Member Author

Tests fail :)

Only because my tests were so good 😛. It's better now - sorry about that. One failure is unrelated.

@nicolas-grekas nicolas-grekas force-pushed the asset-mapper-dev-public-path-cache branch from df81762 to bb97591 Compare May 23, 2023 18:22
@nicolas-grekas
Copy link
Member

Thank you @weaverryan.

@nicolas-grekas nicolas-grekas merged commit cc65825 into symfony:6.3 May 23, 2023
@weaverryan weaverryan deleted the asset-mapper-dev-public-path-cache branch May 23, 2023 20:21
@fabpot fabpot mentioned this pull request May 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
AssetMapper Bug Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
4 participants
@weaverryan @nicolas-grekas @chalasr @carsonbot
0