8000 Suggested change for TreeOp::Put · Issue #12 · turbofish-org/merk · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Suggested change for TreeOp::Put #12

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.

Sign up for GitHub

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

Jump to bottom
Closed
davebryson opened this issue Jul 14, 2019 · 4 comments
Closed

Suggested change for TreeOp::Put #12

davebryson opened this issue Jul 14, 2019 · 4 comments

Comments

@davebryson
Copy link

Not a big deal, but one suggestion I have is to changeTreeOp::Put(&'a[u8]) to TreeOp::Put(Vec<u8>). Why?

  1. The consumer Node.value is already a Vec<u8>
  2. It simplifies TreeOp a bit (lifetime is not needed)
  3. It's easier to use Put(Vec<u8>) from external calls - (again simplifies lifetime stuff)
@mappum
Copy link
Collaborator
mappum commented Jul 14, 2019

That was chosen for performance - I hate unneccessary clones. Would switching to AsRef([u8]) solve this?

@mappum
Copy link
Collaborator
mappum commented Jul 14, 2019

Ah but you're right that it's getting cloned into the node - maybe you're right and we can just change this 👌

@davebryson
Copy link
Author
< 8000 table class="d-block user-select-contain" data-paste-markdown-skip>

Made the change to try it out and see the perf difference. It's in my treeop branch

Bench results (just showing Put related ones):
test bench_put_insert_random: 186,201,818 (&[u8]) --- 168,084,786 (Vec<u8>) ns/iter
test bench_put_insert_sequential: 23,737,681 (&[u8]) --- 24,472,033 (Vec<u8>) ns/iter
test bench_put_update_random: 206,077,496 (&[u8]) --- 194,155,280 (Vec<u8>) ns/iter
test bench_put_update_sequential 25,539,183 (&[u8]) --- 24,806,745 (Vec<u8>) ns/iter

Performance wise they're pretty close. I'll settle for 23,809 random inserts per/sec 😄

Minimal touchpoints to make the change - 👍 to the designer of this thing!

@mappum
Copy link
Collaborator
mappum commented Aug 22, 2019

Closing this because #15 now uses Vec<u8>'s where we would have used &[u8]'s. In the future there might be a few places where we can optimize by using slices, but with a AsRef<[u8]> bound so that we can still accept Vec<u8>.

@mappum mappum closed this as completed Aug 22, 2019
MavenRain added a commit to MavenRain/merk that referenced this issue Jan 20, 2023
@MavenRain @mappum
Update with nomic fork sync (turbofish-org#12)
3d509c8 
* Feature gate jemallocator separately from 'full' feature

* Upgrade dependencies

* Use little-endian 32-bit integers for lengths in kv_hash input encoding

* Use SHA-512/256 hash algorithm instead of Blake 3

* Export MerkSource

* Bump version to 2.0.0

* Add prefixes to KV hash and node hash inputs

* Add large sequential deletion test (was causing stack overflow)

* Recurse into child ops before removing node in Delete op

* Update test hashes for blake3 -> sha2 change

* Format

* Add unit test which tests tree structure after applying recursive delete

* Fix deletion algorithm to ensure we remove correct node after recursing

* Fix warning

* Add repair method

* Add method for recursively getting value from Tree

* Add Snapshot and extract common code from Merk into functions

* Add create_path method to TempMerk

* Export Snapshot

* cargo fmt and we're not using sha2

* Fix hash stuff

* Fix hash tests and clippy

Co-authored-by: Matt Bell <mappum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@davebryson @mappum
0