refactor: merklize into copied pages #652
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
rphmeier
force-pushed
the
rh-copy-pages-merkle
branch
from
43b0bad to
e2190ba
Compare
rphmeier
force-pushed
the
rh-copy-pages-merkle
branch
from
e2190ba to
4c6a8b1
Compare
| TriePosition::new(), | ||
| vec![ | ||
| (key_path![0, 1, 0, 1, 1, 0], val(1)), | ||
| (key_path![0, 1, 0, 1, 1, 1], val(2)), | ||
| ], | ||
| ); | ||
|
|
||
| match walker.conclude() { |
There was a problem hiding this comment.
this test was actually silently broken in master for a couple reasons.
- it broke the contract of the page walker, because it called
advance_and_replacewith a position that was a sub-position of a previousadvance_and_replacecall. - it tested that there were 3 modified pages, however, in the previous version of the test there were only two pages that were being modified (root + 1 child page). the third page came from a broken invariant causing
build_stackto reload a page which was already pushed to theupdated_pagesvector.
the changes in this PR exposed the problems with this test so I fixed them here.
| .map_or(false, |d| d.page_id == child_page_id) | ||
| { | ||
| // UNWRAP: just checked | ||
| self.updated_pages.pop().unwrap() |
There was a problem hiding this comment.
I am fairly certain this is never reachable after the removal of leaf children. The reason this case might've existed before was:
- we begin to replace a leaf at the bottom of a page with 2 leaves at the layer below.
- its leaf children are on the beginning of the next page, so we remove them (page gets pushed onto
updated_pages) - we then traverse
downinto that page and need to get it fromupdated_pages.
However, now we don't have this case. We never delete a page during compaction and then re-enter that page. That's because the parent node of the page must be an internal node, and advance only is done on terminal nodes (leaf/terminator)
There was a problem hiding this comment.
I also agree with that and with the reason why it was previously required. This could be proven by the fact that down() is only called in replace_terminal(..). If it satisfies the fact that all new elements of a subtree are provided in one batch, then there is no way to do what you just explained
There was a problem hiding this comment.
Should we put an unreachable! guard here then?
rphmeier
changed the title
rphmeier
force-pushed
the
rh-deep-clone-fatpage
branch
from
4b9fb4c to
1e5d7f3
Compare
rphmeier
force-pushed
the
rh-copy-pages-merkle
branch
from
4c6a8b1 to
b10f9d8
Compare
| .map_or(false, |d| d.page_id == child_page_id) | ||
| { | ||
| // UNWRAP: just checked | ||
| self.updated_pages.pop().unwrap() |
There was a problem hiding this comment.
I also agree with that and with the reason why it was previously required. This could be proven by the fact that down() is only called in replace_terminal(..). If it satisfies the fact that all new elements of a subtree are provided in one batch, then there is no way to do what you just explained
pepyakin
changed the base branch from
rh-deep-clone-fatpage
to
graphite-base/652
pepyakin
force-pushed
the
rh-copy-pages-merkle
branch
from
b10f9d8 to
a4dd447
Compare
There are two reasons to do this:
commitwhere we have more threads would be best.I suspect that this will actually be faster than
masterfor commit-concurrency > 1. However, I will also benchmark for commit-concurrency = 1 (note: I remove the copies during writeout only further upstack, so the benchmarks will be there)This seems to obviate the read/write pass stuff, so if perf holds up we should likely just remove those in follow-up PRs.