8000
Skip to content

Parallelize host-side hashing and client-side hashing.#169

Merged
BrianPugh merged 1 commit intomainfrom
parallel-hash
Jul 6, 2025
Merged

Parallelize host-side hashing and client-side hashing.#169
BrianPugh merged 1 commit intomainfrom
parallel-hash

Conversation

@BrianPugh
Copy link
Copy Markdown
Owner

@raveslave see if this speeds anything up for you.

@codecov
Copy link
Copy Markdown
codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 53.12500% with 15 lines in your changes missing coverage. Please review.

Project coverage is 70.66%. Comparing base (5e68403) to head (8400dfa).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
belay/device.py 53.12% 7 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   70.65%   70.66%   +0.01%     
==========================================
  Files          39       39              
  Lines        1707     1708       +1     
  Branches      382      384       +2     
==========================================
+ Hits         1206     1207       +1     
  Misses        413      413              
  Partials       88       88              
Flag Coverage Δ
unittests 70.43% <53.12%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jsiverskog
Copy link
Copy Markdown
Contributor

hi,
i can't see any difference in speed with and without this change, unfortunately. they both take ~200-220 ms for sync (when all files have been synced previously, meaning that only the hashing should be performed).

this is from a pc with rp2040.

@BrianPugh
Copy link
Copy Markdown
Owner Author

@jsiverskog without this PR (because it just complicates things) can you profile the code in your setup and see what's taking up all the time? And/or provide a way for me to replicate your setup (I have an rp2040).

@BrianPugh
Copy link
Copy Markdown
Owner Author

@raveslave this didn't happen to speed stuff up for you, did it?

@raveslave
Copy link
Copy Markdown

@jsiverskog you tested a bit right?

@jsiverskog
Copy link
Copy Markdown
Contributor

the only test i did was to compare this with the main branch and didn't see any difference in time. then i made some changes to our code to avoid syncing as often, so the issue is not really that major for us anymore.

however, i should be able to profile this for you @BrianPugh if you're still interested, perhaps later today. is a regular cProfile run enough?

@BrianPugh
Copy link
Copy Markdown
Owner Author

Sure! Anything that helps indicate what the hotspot is for syncing!

@jsiverskog
Copy link
Copy Markdown
Contributor

hi,
sorry for the late reply. attached is a cprofile run where no files are transferred. the list of files in the directory to sync is 8 and it takes ~200 ms for belay to realize that nothing has to be transferred.

stats.zip

@BrianPugh
Copy link
Copy Markdown
Owner Author

so the bottleneck is truly the hashing speed on-device. It would be cool if we could add a check like "if size & mtime match the current on-computer files, then skip hashing and assume that they are the same."

My fnv1a32 native module is around 3.5x faster, which could make this fast enough that it feels fine; i'll see if i can make a PR doing that.

All of that said, I don't think this PR harms anything, so I think i'll merge it as it could result in speedups under certain situations (but it doesn't make a meaningful difference in the situations described so far)

@BrianPugh BrianPugh merged commit 62f82d6 into main Jul 6, 2025
@BrianPugh BrianPugh deleted the parallel-hash branch July 6, 2025 23:31
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.

3 participants

0