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

Allocation pool the chunked iters in InstanceEnv #2038

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Dec 5, 2024

Description of Changes

This adds a ChunkPool which ChunkedWriter uses.
Also removes some dead code in InstanceEnv.

Based on flame graphs and timings, I'm not sure this changes much :/
We might want to try a different approach that doesn't do any chunking at all but rather writes to a singe buffer that we pool.
We can then keep an offset when iterating instead. Not sure how this will work with row boundaries though.

I measured things again for .iter(), and in this case, it seems like the time spent in datastore_table_scan_bsatn decreased by a few %. Not a huge amount, but still an improvement. The index scan bit not improving makes sense -- there's a unique index there and only a single element is fetched.

Fixes #2020.

@Centril Centril requested a review from coolreader18 December 5, 2024 18:18
Copy link
Collaborator

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

LGTM. Do you know if we have any tests that actually exercise the chunk.len() > ROW_ITER_CHUNK_SIZE codepath? That feels like it'd be good to have if not especially as this gets optimized more.

@Centril
Copy link
Contributor Author

Centril commented Dec 9, 2024

Do you know if we have any tests that actually exercise the chunk.len() > ROW_ITER_CHUNK_SIZE codepath? That feels like it'd be good to have if not especially as this gets optimized more.

No sorry, I can neither confirm nor deny the existence of such tests. 😊

@Centril Centril added this pull request to the merge queue Dec 9, 2024
Merged via the queue into master with commit d8154e7 Dec 9, 2024
12 checks passed
@Centril Centril deleted the centril/pool-chunked-iters branch December 9, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: pool chunks used by row iterators in InstanceEnv
2 participants