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

Serialized storage state can potentially grow without bound #661

Closed
hansonw opened this issue Oct 20, 2017 · 3 comments · Fixed by #738
Closed

Serialized storage state can potentially grow without bound #661

hansonw opened this issue Oct 20, 2017 · 3 comments · Fixed by #738
Labels
atom-fs Filesystem-related issues to address when developing a stable release of the `atom-fs` module. performance Issues with the package's speed or CPU usage, including startup time.

Comments

@hansonw
Copy link
Contributor

hansonw commented Oct 20, 2017

Hey, we've come across some situations where users with large projects end up with a file-icons cache larger than 1MB (serialized as JSON).

This ends up causing some pretty severe slowdown, as Atom typically serializes its state whenever idle (usually every couple of seconds), leading to a lot of GC pressure.

Would it make sense to impose some kind of limit on the size of the Storage class? Potentially turning it into an LRU cache?

Thanks!

@Alhadis
Copy link
Member

Alhadis commented Oct 22, 2017

Hey @hansonw,

Stale data is most likely to be the main culprit here. References to outdated files aren't checked or cleared when they should be, which is something I've been meaning to implement once I resume work on atom-fs.

However, the LRU approach wouldn't be an acceptable solution because of the way it's used at startup: when determining which icon to use, the package consults the Storage class for sm existing icon. This stops a distracting "flicker" for files with hashbangs or modelines - for projects where the LRU files were the first ones visible when the project was opened, this kind of defeats the purpose.

Ultimately, this all comes down to the need to progress with atom-fs's development, something which I've delayed until I've finished something else I'm working on.

@Alhadis Alhadis added the atom-fs Filesystem-related issues to address when developing a stable release of the `atom-fs` module. label Oct 22, 2017
@hansonw
Copy link
Contributor Author

hansonw commented May 17, 2018

Circling back to this one as some users at Facebook are frequently running into storage sizes of tens of thousands of files (probably due to our large repository sizes). This causes Atom to perform very poorly since it's attempting to serialize the storage values every couple of seconds.

I've identified #737 as a possible exacerbating factor (causing resources to be created on VCS events aside from just regular use) but it seems like it would be a good idea to set at least some kind of bound as a sanity check?

I understand the problem with flickering icons at startup but it's certainly preferable to causing Atom's performance to degrade. What kind of upper bound would you be comfortable with? With some rough testing, storing ~10000 files in storage takes about 1MB so we'd be fine with something like that.

We pre-install file-icons for all Facebook users and people generally like it a lot; it'd be a shame to have to disable it for performance issues :)

@Alhadis
Copy link
Member

Alhadis commented May 17, 2018

These issues are why I haven't been sleeping at night. 😉

~10,000 sounds like a reasonable limit for now. I'm willing to accept a PR to add it. 😉

@Alhadis Alhadis added the performance Issues with the package's speed or CPU usage, including startup time. label Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atom-fs Filesystem-related issues to address when developing a stable release of the `atom-fs` module. performance Issues with the package's speed or CPU usage, including startup time.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants