Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Delete values immediately from DB overlay #1631

Merged
merged 3 commits into from Jul 16, 2016
Merged

Delete values immediately from DB overlay #1631

merged 3 commits into from Jul 16, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Jul 15, 2016

About 20% block import speed increase

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Jul 15, 2016
@coveralls
Copy link

coveralls commented Jul 15, 2016

Coverage Status

Coverage increased (+0.05%) to 76.421% when pulling c145764 on db-purge-fix into d14b687 on master.

@rphmeier
Copy link
Contributor

This doesn't add entries of the form (key, -1) like remove did when there is no entry for the key

@gavofyork
Copy link
Contributor

tests shouldn't be passing; this needs fixing and a test to ensure it's caught.

@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 16, 2016
@arkpar
Copy link
Collaborator Author

arkpar commented Jul 16, 2016

OverlayRecentDB does not keep deleted key-values in memory. All calls to remove were followed by purge. This logic did not change with this PR

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jul 16, 2016
@arkpar
Copy link
Collaborator Author

arkpar commented Jul 16, 2016

This is a pruning overlay for the journaled states, not to be confused with transaction overlay.

@gavofyork
Copy link
Contributor

ok, however, the function name should change (since it's not strictly equivalent to remove + purge), the documentation should be made abundantly clear that it has subtly different semantics (since this could very easily introduce very difficult to detect bugs if a later accidental replacement were to happen that depended on remove_and_purge + insert giving a 0 ref-count) and it should be demonstrated at the call site that the refs is always > 0.

there should also be an assertion to the same effect in the function.

@arkpar
Copy link
Collaborator Author

arkpar commented Jul 16, 2016

How is it not equivalent? The function does what the name says, removes a reference count and purges it if it reaches zero or negative.

remove();
purge();
insert();

is equivalent to

remove_and_purge();
insert();

for a particular key of course

@gavofyork
Copy link
Contributor

gavofyork commented Jul 16, 2016

If the key is not in the DB at the beginning, the semantics are different.

  • remove will insert a key of refs -1 (L238);
  • purge will leave it in there (L119);
  • insert will increase it back to 0.

whereas:

  • remove_and_purge will do nothing;
  • insert will insert with a reference count of 1.

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 16, 2016
@arkpar
Copy link
Collaborator Author

arkpar commented Jul 16, 2016

I see, will change remove_and_purge to match purge behaviour and add a test

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jul 16, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 16, 2016
@coveralls
Copy link

coveralls commented Jul 16, 2016

Coverage Status

Coverage increased (+0.03%) to 76.403% when pulling 351fadc on db-purge-fix into d14b687 on master.

@coveralls
Copy link

coveralls commented Jul 16, 2016

Coverage Status

Coverage decreased (-0.02%) to 76.355% when pulling 351fadc on db-purge-fix into d14b687 on master.

@arkpar arkpar merged commit 6441759 into master Jul 16, 2016
@arkpar arkpar deleted the db-purge-fix branch July 28, 2016 08:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants