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

Change children hash function to XOR #7

Merged
merged 1 commit into from
Feb 5, 2021
Merged

Conversation

systream
Copy link

To improve the performance of the inserts, I have changed children hash function. With XOR not needed to recalculate the whole children hash, so it is 6-8 times faster and it does not break any test.

It is highly inspired what guys did in RIAK 3.0

@ferd ferd self-assigned this Jan 22, 2021
@ferd
Copy link
Owner

ferd commented Jan 29, 2021

Do you have a link to the Riak 3.0 work? I understand how this one should work, but it seems like a significant-enough change that I'd not trust my tests to actually be that good to uncover weird edges. If other people have done the analysis work, that'd be useful for me to read.

@systream
Copy link
Author

They mention it in the documentation.
For example: https://github.com/martinsumner/kv_index_tictactree/blob/develop-3.0/docs/TICTAC.md#alternative-merkle-tree-tictac

The only scenario where I think it could be problems when hash collision occurs.
For example hash(K1), and hash(K2), hash(K3) returns the same result (let's call it Hash1).
Hash1 bxor Hash1 bxor Hash1 will return Hash1.
This could happen:
hash(K1) =:= hash(K1) bxor hash(K2) bxor hash(K3).

With the original hash([hash(K1), hash(K2), hash(K3)]) it will generates a different uniq hash.
hash([hash(K1)]) =/= hash([hash(K1), hash(K2), hash(K3)]),
but for example it may could happen hash([hash(k1)]) =:= hash([hash(k1), hash(k3)])

This same weird behaviour can happen (hash collision) with the leafs or also could happen when hashing the child's hashes. I thing the current implementation has almost the same probability of this kind of errors.

@ferd
Copy link
Owner

ferd commented Jan 30, 2021

Makes sense. I'm likely to merge this, but will probably have to bump to a next major version since someone storing a serialized tree would see it break across versions.

@ferd ferd merged commit edde25f into ferd:master Feb 5, 2021
@ferd
Copy link
Owner

ferd commented Feb 5, 2021

MErged & Cut a new release, along with a hex publish. Thanks!

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.

2 participants