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

Batch update. #110

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

Batch update. #110

wants to merge 126 commits into from

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Jul 31, 2020

I open this draft PR to gather opinion on how to solve #109.

This is a branch I wrote, but I drop it because the performance gains are not relevant (iirc some 20/30% on memorydb bench which is pointless when considering the additional code).

Yet doing it with this kind of method (sorted input and only a stack of branch in memory), could help with #109 .

So is it a good idea to push forward with this?

It requires some code cleaning, variable renaming, removing attach and dettach code, asserting the proof is optimal.
Maybe removing the ProcessingStack abstraction or rewriting it from scratch could result in more compact code (looking back at it is probably around 1000 line of code without the attach/detach and test specific code).
There is also some redundant node accessor and other not really good looking stuff (main purpose of initial implementation was only to get an idea of the performance improvment).

Note that the attach/detach subtrie code was written for a possible optimisation of clear_prefix substrate operation that would just detach a trie and only parse/delete its value when pruning (that was proposed for child trie).

So this PR is mainly here to illustrate the second solution to #109 , I would be happy to continue this, but maybe it is better to write the same anew.

Note that this originally replies to #40.

update test, failing for simple leaf value update.
Next extend case, and then case where branch in middle of partial.
of a partial (currently it does as if it is always append only).
Condition is duplicated and should be refactored.
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.

1 participant