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

Add simple deletion #16

Merged
merged 9 commits into from
Apr 25, 2019
Merged

Add simple deletion #16

merged 9 commits into from
Apr 25, 2019

Conversation

christianbundy
Copy link
Member

  • Add .gitignore and update deps
  • Add simple del method to overwrite data with null bytes
  • Use non-deprecated buffer methods
  • Remove unused variables

@christianbundy
Copy link
Member Author

Backlink: depends on flumedb/aligned-block-file#5

@christianbundy christianbundy requested review from arj03 and regular April 11, 2019 18:24
@arj03
Copy link
Contributor

arj03 commented Apr 11, 2019

I read the commit, how does this affect performance? I see that stream now filters all elements, so I'm wondering what kind of overhead this might or might not have?

@christianbundy
Copy link
Member Author

Thanks for the review! I took a look at the performance and noticed that isDeleted() was super slow because it was allocating an empty buffer and checking for equality. I've pushed a commit that resolves the performance regression, so the benchmarks look like:

master

validate 100000 974 8695.652173913044 11.5
flume 100000 10000 34352.45620061834 2.911
minimal 100000 2446 7200.979333189313 13.887
legacy 100000 436 4760.544606302961 21.006
random-read 100000 10000 59453.032104637336 1.682
random-read 100000 0 59453.032104637336 1.682

1351d03 (slow)

validate 100000 1541 8167.932696234583 12.243
flume 100000 10000 31735.9568390987 3.151
minimal 100000 2889 6796.71039217019 14.713
legacy 100000 851 4409.171075837742 22.68
random-read 100000 10000 49726.504226752855 2.011
random-read 100000 0 49726.504226752855 2.011

32a1392

validate 100000 969 8604.371020478404 11.622
flume 100000 10000 34094.78349812479 2.933
minimal 100000 2779 7303.534910896874 13.692
legacy 100000 600 4753.981459472308 21.035
random-read 100000 10000 56338.02816901409 1.775
random-read 100000 0 56338.02816901409 1.775

@arj03
Copy link
Contributor

arj03 commented Apr 13, 2019

Excellent @christianbundy! The commit looks good, I'm not sure if blanking is the best option, havn't thought deeply enough about that, but I trust you on that.

I really appreciate you driving deletion forward!

@christianbundy
Copy link
Member Author

Thanks @arj03! If you think of any other techniques I'd be happy to hear them, this shouldn't require a migration or anything so we can always change the implementation if we come up with something clever.

I've got a call with Dominic in a bit, I'm going to check in with him before merging just in case. 👍

@christianbundy
Copy link
Member Author

Oops, this slipped my mind on the call -- any changes you'd like to see @dominictarr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants