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

[CLEANUP] Remove a redundant CSS data cache #1018

Merged
merged 1 commit into from
Apr 14, 2021
Merged

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Apr 14, 2021

The cache with the key CACHE_KEY_CSS was storing all of the parsed CSS data
as a single entry, but would only ever be set once and never accessed. It does
not make sense to call inlineCss again on the same document with the same CSS,
but in any case this method clears all caches as its first action.

This will reduce the memory footprint after inlineCss returns.

The cache with the key `CACHE_KEY_CSS` was storing all of the parsed CSS data
as a single entry, but would only ever be set once and never accessed.  It does
not make sense to call `inlineCss` again on the same document with the same CSS,
but in any case this method clears all caches as its first action.

This will reduce the memory footprint after `inlineCss` returns.
@JakeQZ JakeQZ added this to the 6.0.0 milestone Apr 14, 2021
@JakeQZ JakeQZ self-assigned this Apr 14, 2021
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Apr 14, 2021

It's possible that this cache was once useful, but as the code has changed over the years it has become redundant.

It could be made useful by making it static and not having inlineCss clear it. However, that would result in a permanent memory footprint after Emogrifier has been used, unless explicitly cleared by the client, which would probably put unnecessary onus on the client.

@oliverklee oliverklee merged commit b2004a4 into main Apr 14, 2021
@oliverklee oliverklee deleted the cleanup/redundant-cache branch April 14, 2021 11:23
JakeQZ added a commit that referenced this pull request Apr 19, 2021
Entry for #1018 was added at bottom of list, but entries should be in
reverse-chronological order.
oliverklee pushed a commit that referenced this pull request Apr 20, 2021
Entry for #1018 was added at bottom of list, but entries should be in
reverse-chronological order.
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.

None yet

2 participants