cristianbote / goober

🥜 goober, a less than 1KB 🎉 css-in-js alternative with a familiar API
https://goober.rocks
MIT License
3.14k stars 119 forks source link

Unbounded global cache can potentially grow very large during static SSR #269

Open danielweck opened 3 years ago

danielweck commented 3 years ago

When using Preact render-to-string, Goober's cache grows indefinitely. There is a "global" object (well, module scoped) used to memoize "compiled object" -> "classname/hash" and "classname/hash" -> "generated CSS".

Code references:

https://github.com/cristianbote/goober/blob/942ab335d5fd4f125d0ae9e340b7004a45b52171/src/core/hash.js#L6-L9

https://github.com/cristianbote/goober/blob/942ab335d5fd4f125d0ae9e340b7004a45b52171/src/core/hash.js#L41-L42

https://github.com/cristianbote/goober/blob/942ab335d5fd4f125d0ae9e340b7004a45b52171/src/core/hash.js#L50-L54

I personally have not hit any performance bottleneck as my project is quite small, but I thought I'd raise the issue.

This reminds me of the recent commit revert in Preact render-to-string (similar memoization / unbounded cache problem):

https://github.com/preactjs/preact-render-to-string/issues/191

https://github.com/preactjs/preact-render-to-string/pull/187/files

cristianbote commented 3 years ago

Hey @danielweck! Thanks for opening this one. Love your findings.

Indeed, the cache object can grow depending on your usage and styled/css components. Initially cache was bound with an entry count. I believe it was 10k entries or something but ultimately got removed because all the test/website data I could get my hands on pointed that it wasn't needed since the runtime will get at a point where everything will get cached. So the entry counter and cache access were adding some penalty instead. So I removed them. Also it took up space without a huge reward.

But if the community or folks using it stumble on an out of memory issue, of course, needs to be addressed asap.

zhaoyao91 commented 3 years ago

maybe the cache could be shifted with the css function instance? so there would be no memory leak since its lifecycle is with the css function instance.

danielweck commented 3 years ago

FYI insightful cache / hash -related comments in this PR by @kitten https://github.com/cristianbote/goober/pull/288#issue-615675197