Skip to content

Allocate pages using a mult-tenant lock-free pool #2587

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Apr 11, 2025

Description of Changes

Fixes #2016.

This PR implements page allocation through a lock-free multi-tenant pool,
using crossbeam_queue::ArrayQueue as the basis of the pool.
This lock-free queue is bounded. And this PR sets that bound to to 32 GiB. After that, pages will be dropped.

API and ABI breaking changes

None

Expected complexity level and risk

2

Testing

Most of the testing is done by adjusting existing tests.

@Centril Centril force-pushed the centril/page-pool branch 2 times, most recently from 9bf77a5 to 925547e Compare April 11, 2025 14:01
@@ -433,6 +433,7 @@ impl<P: BlobProvider> SnapshotFetcher<P> {
}

fn verify_page(&self, expected_hash: blake3::Hash, buf: &[u8]) -> Result<()> {
// TODO(centril, kim): consider whether we want to use the page pool here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @kim

@Centril Centril marked this pull request as ready for review April 11, 2025 14:31
@Centril Centril requested a review from gefjon April 11, 2025 14:31
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Minor nits. Looks good! Would love some benchmark results and a bot test.

@gefjon
Copy link
Contributor

gefjon commented Apr 11, 2025

benchmarks please

Copy link

github-actions bot commented Apr 11, 2025

Criterion benchmark results

Error when comparing benchmarks: Couldn't find AWS credentials in environment, credentials file, or IAM role.

Caused by:
Couldn't find AWS credentials in environment, credentials file, or IAM role.

Copy link

Callgrind benchmark in progress...

1 similar comment
Copy link

Callgrind benchmark in progress...

Copy link

github-actions bot commented Apr 11, 2025

Criterion benchmark results

Error when comparing benchmarks: Couldn't find AWS credentials in environment, credentials file, or IAM role.

Caused by:
Couldn't find AWS credentials in environment, credentials file, or IAM role.

Copy link
Contributor

@jsdt jsdt left a comment

Choose a reason for hiding this comment

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

This looks good. My main comment is about adding metrics to gauge how useful this is.

@bfops bfops added the release-any To be landed in any release window label Apr 14, 2025
@Centril Centril force-pushed the centril/page-pool branch from 1e09780 to 37d500f Compare April 17, 2025 11:14
@Centril Centril requested a review from jsdt April 17, 2025 15:04
@Centril Centril removed their assignment Apr 17, 2025
@jsdt jsdt assigned Centril and unassigned jsdt Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: store a pool of Pages rather than creating a new one each time
4 participants