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

provider: add a buffered KeyChanFunc. #870

Merged
merged 4 commits into from
Mar 7, 2025

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Mar 5, 2025

This provider helper allows to buffer all the results from a keyChanFunc in memory.

The purpose is to fix issues with slow re-providing. In the case of ipfs/kubo#10596, the slow re-providing process causes starvation of any operations trying to read/write to the pinset.

With the new buffered KeyChanFunc, we can read everything we need to announce into memory first, and free any locks as soon as possible.

Given the compact size of CIDs (<50bytes), I don't think any complexer approach (batch reading and lock/unlock handling) is justified. People with pinsets of 20 million items that want to announce everything can well spare an extra GB of RAM. For smaller repos the impact becomes negligible.

The test targets precisely the use-case and ensures we don't starve operations while interacting with dspinner.

This provider helper allows to buffer all the results from a keyChanFunc in memory.

The purpose is to fix issues with slow re-providing. In the case of
ipfs/kubo#10596, the slow re-providing process
causes starvation of any operations trying to read/write to the pinset.

With the new buffered KeyChanFunc, we can read everything we need to announce
into memory first, and free any locks as soon as possible.

Given the compact size of CIDs (<50bytes), I don't think any complexer approach (batch reading and lock/unlock handling) is justified. People with pinsets of 20 million items that want to announce everything can well spare an extra GB of RAM. For smaller repos the impact becomes negligible.

The test targets precisely the use-case and ensures we don't starve operations while interacting with dspinner.
@hsanjuan hsanjuan self-assigned this Mar 5, 2025
@hsanjuan hsanjuan requested a review from a team as a code owner March 5, 2025 12:17
Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

The reasoning and the code make sense. The windows CI doesn't seem happy with the tests though

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Mar 5, 2025

yeah @gammazero is this related to dequed "power of two" grow allocations?

@gammazero
Copy link
Contributor

gammazero commented Mar 5, 2025

is this related to dequed "power of two" grow allocations?

I do not think so. It does not look like your test creates such a huge number of pins that it should cause OOM. Besides, the grow by power of two is what a slice does anyway. I will test this locally and see what it eating up all the memory.

The deque is very memory efficient when inflow and outflow are somewhat balanced, i.e. when size changes do not incur many queue resizes. However, if the use pattern is to enqueue a large (relative to the base capacity) amount of data and then read all of the data, then a better deque implementation is this: https://github.com/edwingeng/deque. It really depends on the use pattern which is more efficient.

@hsanjuan hsanjuan requested a review from gammazero March 6, 2025 13:53
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Mar 6, 2025

The test failure was due to recursion when ipld-prime traverses a 4092-deep graph.

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

See comment.

@hsanjuan hsanjuan merged commit 93ea580 into main Mar 7, 2025
13 checks passed
@hsanjuan hsanjuan deleted the feat/buffered-provider-KeyChanFunc branch March 7, 2025 07:36
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.

3 participants