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

SubtreeCache: Batch tile reads when writing nodes #2589

Merged
merged 7 commits into from
Jul 27, 2021

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Jul 21, 2021

The SubtreeCache uses two types of "read" callbacks: GetSubtreeFunc for
single-tile reads, and GetSubtreesFunc for batched reads. Besides being
complex, this also has performance issues: the single-node SetNodeHash
method fetches only one tile using GetSubtreeFunc, so a typical
SetMerkleNodes call results in a series of individual reads.

The only use-case for node writes in Trillian (the sequencer) does exactly one
SetMerkleNodes call per transaction with the entire write set, so there is no
need to sub-divide it into single-node writes in SubtreeCache.

This change replaces the SetNodeHash function with a batched SetNodes so
that the corresponding reads are batched too, and the single-tile callback is no
longer needed and removed.

#2378

Checklist

@pav-kv pav-kv requested a review from a team as a code owner July 21, 2021 17:22
@google-cla google-cla bot added the cla: yes label Jul 21, 2021
@pav-kv
Copy link
Contributor Author

pav-kv commented Jul 21, 2021

Review commit-by-commit might make sense.

@pav-kv pav-kv force-pushed the rm_single_tile_fetch branch from 17adc67 to 2e73d7f Compare July 22, 2021 00:14
@pav-kv
Copy link
Contributor Author

pav-kv commented Jul 23, 2021

PTAL

1 similar comment
@pav-kv
Copy link
Contributor Author

pav-kv commented Jul 26, 2021

PTAL

@pav-kv
Copy link
Contributor Author

pav-kv commented Jul 26, 2021

Thanks Al.

@Martin2112 It would be nice to have a second pair of eyes on it too.

@pav-kv pav-kv force-pushed the rm_single_tile_fetch branch from 95dd136 to 326dda9 Compare July 27, 2021 10:36
Copy link
Contributor Author

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

Addressed Martin's comments.

storage/cache/subtree_cache.go Show resolved Hide resolved
storage/cache/subtree_cache.go Show resolved Hide resolved
storage/cache/subtree_cache.go Show resolved Hide resolved
@pav-kv pav-kv merged commit 30fa128 into google:master Jul 27, 2021
@pav-kv pav-kv deleted the rm_single_tile_fetch branch July 27, 2021 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants