Refactor pool logic #38
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukevalenta
force-pushed
the
lvalenta/pool-cleanup
branch
from
0b55e86 to
56fdac7
Compare
* Clean up AddLeafResult enum and add source() method to replace separate AddLeafResultSource enum * Move majority of logic for add_leaf_to_pool to mthods of PoolState. This will allow us to more easily change how the pooling works in a future PR (e.g., #33). * Remove pool_size parameter from configuration. It's never been useful to customize per log, so better to just make it a constant.
lukevalenta
force-pushed
the
lvalenta/pool-cleanup
branch
from
56fdac7 to
d3932b9
Compare
- Move Pool struct fields into PoolState, renaming by_hash to pending and done to pending_done. - Add take() and reset() functions to PoolState to mutate state - Rename pending_leaves -> pending_entries, sequence_pool -> sequence_entries
lukevalenta
force-pushed
the
lvalenta/pool-cleanup
branch
from
d3932b9 to
8ff3f8e
Compare
PendingLogEntry contains all of the fields of LogEntry except for the leaf index and timestamp.
lukevalenta
force-pushed
the
lvalenta/pool-cleanup
branch
from
8ff3f8e to
4568c22
Compare
Instead of using a Sender per pool of entries, allocate one Sender per pending entry. This is slightly less efficient as we're creating more channels, but gives much more flexibility for how we handle individual entries. More thorough benchmarking is needed, but from some quick tests there was no noticeable difference. This change also allows us to stop tracking the 'pool_index' of each log entry, and just transmit the final index on each entry's channel. This refactor will make it easier to only select a subset of the pending entries each time sequencing occurs, so we can try to keep the tree as a multiple of the full tile width (256), so we create fewer partial data and level-0 tiles (#33).
…ry to avoid clone
lukevalenta
force-pushed
the
lvalenta/pool-cleanup
branch
from
07b1cc1 to
ccc52f0
Compare
Add parameter specifying the maximum number of times a pending entry might be held back to prefer creating full tiles instead of partial tiles. fixes #33 This change required moving the logic to load the SequenceState from the sequence_entries function to the wrapping sequence function, and updating tests accordingly.
lukevalenta
force-pushed
the
lvalenta/pool-cleanup
branch
from
ccc52f0 to
2af4060
Compare
rozbb
approved these changes
May 21, 2025
Contributor
rozbb
left a comment
rozbb
left a comment
There was a problem hiding this comment.
Looks good! Just some small notes
| // Flush all of the leftover entries if the oldest is before the cutoff. | ||
| let flush_oldest = self.holds >= max_pending_entry_holds; | ||
|
|
||
| if leftover == 0 || flush_oldest { |
Contributor
There was a problem hiding this comment.
So if flush_oldest is triggered when there's 1.5 tiles pending, we'll publish 1.5 files? Why not just publish a full one and hold back the half?
Contributor
Author
There was a problem hiding this comment.
Yep, that's what will currently happen, unless max_pending_entry_holds = 0, in which case we always flush the oldest (which is the previous behavior before this PR).
This test demonstrates that behavior: https://github.com/cloudflare/azul/pull/38/files#diff-04abdf856a7b917bbd1ca592d4f3026c5f36bd03afcc4928103cf5a58143670bR1509.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains quite a bit of refactoring of the pool logic in order to address #33. The most significant change is using a separate https://docs.rs/tokio/latest/tokio/sync/watch/index.html
{Sender, Receiver}pair per entry instead of per "pool" of entries. This gives us the flexibility to break up pools of pending entries so we can hold back some entries when sequencing to try to prefer creating full tiles.The commits can be reviewed independently and each have more detailed commit messages, although some of the commits remove or refactor code added in previous commits (e.g., the
Poolstruct).