data61 / clkhash

CLK hash: hash pii for entity matching

Apache License 2.0

47 stars 9 forks source link

Feature allow thread pool executor #423

Closed hardbyte closed 3 years ago

hardbyte commented 3 years ago

As discussed in https://github.com/data61/clkhash/pull/340, this closes #339.

codecov[bot] commented 3 years ago

Codecov Report

Merging #423 (0d72a4e) into master (8e14eb6) will decrease coverage by 0.26%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
- Coverage   94.44%   94.17%   -0.27%     
==========================================
  Files          12       12              
  Lines         936      945       +9     
==========================================
+ Hits          884      890       +6     
- Misses         52       55       +3

hardbyte commented 3 years ago

Could be worth testing, I had assumed that the bulk of the work would be in the low level crypto calls (HMAC etc) and that they would be releasing the GIL which would allow threads to still be useful.

max_workers argument would be a nicer api though so happy to implement that

hardbyte commented 3 years ago

You were correct, it is faster using no threads.

Informal numbers:

Passing max_workers=4 to ProcessPool ~ 10kCLK/s
passing max_workers=8 to ProcessPool ~12kCLK/s
Using default ThreadPool ~1.9 kCLK/s
Main loop (no executor) ~4kCLK/s Replacement PR here - https://github.com/data61/clkhash/pull/424