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

lib/model: Change weak hash algo, add heuristic #3872

Closed

Conversation

AudriusButkevicius
Copy link
Member

No description provided.

Copy link
Member

@calmh calmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM

@@ -1216,7 +1220,7 @@ func (f *sendReceiveFolder) copierRoutine(in <-chan copyBlocksState, pullChan ch
f.model.fmut.RUnlock()

var weakHashFinder *weakhash.Finder
if !f.DisableWeakHash {
if (len(state.file.Blocks)-state.have)*100/len(state.file.Blocks) >= f.WeakHashThresholdPct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this expression should definitely be a variable of some sort with a name

Copy link
Member

@calmh calmh Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, potential div by zero for empty files?

@calmh
Copy link
Member

calmh commented Jan 4, 2017

[RJBTF] 21:27:44 INFO: Single thread hash performance is 345 MB/s using minio/sha256-simd (345 MB/s using crypto/sha256).
...
[RJBTF] 21:27:58 VERBOSE: Scanning folder "default", 9% done (218.0 MiB/s)

still quite a lot of hashing performance we are losing here. Slightly more than the theoretical 1 / (1/345 + 1/1675) = 286 (the 1675 is the adler32 speed on my computer using the benchmark).

By not adding the whf to the multiwriter I get 256 MB/s, so it's not too far of the expected performance I guess.

@AudriusButkevicius
Copy link
Member Author

I guess we could go for xxhash, which would potentially raise that a bit, but then the rolling would be incredibly expensive

@@ -1216,7 +1220,8 @@ func (f *sendReceiveFolder) copierRoutine(in <-chan copyBlocksState, pullChan ch
f.model.fmut.RUnlock()

var weakHashFinder *weakhash.Finder
if !f.DisableWeakHash {
blocksPercentChanged := (len(state.file.Blocks) - state.have) * 100 / len(state.file.Blocks)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about len(state.file.Blockes) == 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -1216,7 +1220,12 @@ func (f *sendReceiveFolder) copierRoutine(in <-chan copyBlocksState, pullChan ch
f.model.fmut.RUnlock()

var weakHashFinder *weakhash.Finder
if !f.DisableWeakHash {
blocksPercentChanged := 0
if len(state.file.Blocks) != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if tot := len(state.file.Blocks); tot > 0 {
    blocksPercentChanged = (tot - state.have) * 100 / tot
}

🚲 🏡

@calmh
Copy link
Member

calmh commented Jan 4, 2017

This is a vast improvement on both sides, it's good enough for now.

@AudriusButkevicius
Copy link
Member Author

🚂 🚂

@calmh
Copy link
Member

calmh commented Jan 4, 2017

@st-review merge it yo

lib/model, lib/weakhash: Hash using adler32, add heuristic in puller

Adler32 is much faster, and the heuristic avoid the obvious cases where it will not help.

@st-review
Copy link

@calmh: Build status is pending. I'll wait until it goes green and then merge!

@calmh
Copy link
Member

calmh commented Jan 4, 2017

obtypo in the commit message, which will look like you did it 💯 😑

@st-review
Copy link

👌 Merged as 29d010e. Thanks, @AudriusButkevicius!

@st-review st-review closed this Jan 4, 2017
st-review pushed a commit that referenced this pull request Jan 4, 2017
Adler32 is much faster, and the heuristic avoid the obvious cases where it
will not help.

GitHub-Pull-Request: #3872
@AudriusButkevicius AudriusButkevicius deleted the weakhash branch January 4, 2017 22:03
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jan 5, 2018
@syncthing syncthing locked and limited conversation to collaborators Jan 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants