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

fix: retrieve from memory when waiting for snapshots #5161

Merged
merged 1 commit into from
Dec 18, 2024
Merged

fix: retrieve from memory when waiting for snapshots #5161

merged 1 commit into from
Dec 18, 2024

Conversation

sprutton1
Copy link
Contributor

@sprutton1 sprutton1 commented Dec 18, 2024

The original implementation of these calls went directly to the memory cache, knowing that we were waiting for something new and it couldn't be on the disk yet. When foyer was added, this changed to go directly to the cache, meaning that we are knowingly spawning a thread that reads from disk with every attempt here, every millisecond, for up to 2000 milliseconds. This creates a TON of disk pressure and additional allocations. I think this might be part of the source of our memory pressure issues, but this really leads me to believe that there is a leak internal to foyer around how they load things from the disk (maybe specifically around misses?).

Another curious thing is that I don't seem to have any traces that point to the disk misses, of which there should be many thousand.

Not that this undoes what was done earlier to move us to a just-memory cache. I would like to see how this performs in tools prod overnight at least to see if it has a significant impact on the memory allocations.

@sprutton1 sprutton1 marked this pull request as ready for review December 18, 2024 21:07
@sprutton1
Copy link
Contributor Author

/try

Copy link

github-actions bot commented Dec 18, 2024

Okay, starting a try! I'll update this comment once it's running...\n
🚀 Try running here! 🚀

Copy link
Contributor

@zacharyhamm zacharyhamm left a comment

Choose a reason for hiding this comment

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

The changes to the read wait are great imo. I like using the &str better, but it's not a blocker.

@sprutton1 sprutton1 added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit da56846 Dec 18, 2024
26 checks passed
@sprutton1 sprutton1 deleted the mem branch December 18, 2024 23:11
@sprutton1
Copy link
Contributor Author

The changes to the read wait are great imo. I like using the &str better, but it's not a blocker.

@zacharyhamm
Just a style preference or something else?

@zacharyhamm
Copy link
Contributor

The changes to the read wait are great imo. I like using the &str better, but it's not a blocker.

@zacharyhamm

Just a style preference or something else?

&str means you can pass a String as easily as a &str or an Arc, and means you don't have to do a ref count operation in the read loop

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.

2 participants