-
-
Notifications
You must be signed in to change notification settings - Fork 840
Remove the complicated block search logic for a simpler branchless #1124
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1124 +/- ##
=======================================
Coverage 89.50% 89.51%
=======================================
Files 203 203
Lines 20231 20201 -30
=======================================
- Hits 18107 18082 -25
+ Misses 2124 2119 -5
Continue to review full report at Codecov.
|
PSeitz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler and faster, really cool. One minor comment. Also I don't know if the data is monotonically increasing or if we can have duplicates.
src/postings/block_search.rs
Outdated
| 1041, 1048, 1061, 1062, 1069, 1072, 1079, 1082, 1083, 1102, 1140, 1165, 1170, 1176, | ||
| 1189, 1195, 1198, 1200, 1201, 1208, 1223, 1240, 1252, 1276, 1307, | ||
| ]; | ||
| for target in block.iter().flat_map(|&el| vec![el - 1, el].into_iter()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add
vec![el - 1, el, el+1] to test with a jump size larger than 1 to the next bucket (except the last element .take(block.len() * 3 -1) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
el +1 is the same as el-1 for the precedent element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll test all elements in [0..1308).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler and faster, really cool. One minor comment. Also I don't know if the data is monotonically increasing or if we can have duplicates.
Good point I modified the contract. There was an error in it too.
binary search The code is simpler and faster. Before test postings::bench::bench_segment_intersection ... bench: 2,093,697 ns/iter (+/- 115,509) test postings::bench::bench_skip_next_p01 ... bench: 58,585 ns/iter (+/- 796) test postings::bench::bench_skip_next_p1 ... bench: 160,872 ns/iter (+/- 5,164) test postings::bench::bench_skip_next_p10 ... bench: 615,229 ns/iter (+/- 25,108) test postings::bench::bench_skip_next_p90 ... bench: 1,120,509 ns/iter (+/- 22,271) After test postings::bench::bench_segment_intersection ... bench: 1,747,726 ns/iter (+/- 52,867) test postings::bench::bench_skip_next_p01 ... bench: 55,205 ns/iter (+/- 714) test postings::bench::bench_skip_next_p1 ... bench: 131,433 ns/iter (+/- 2,814) test postings::bench::bench_skip_next_p10 ... bench: 478,830 ns/iter (+/- 12,794) test postings::bench::bench_skip_next_p90 ... bench: 931,082 ns/iter (+/- 31,468)
binary search
The code is simpler and faster.