-
Notifications
You must be signed in to change notification settings - Fork 53
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
Improve RadixNode add a few more benchmarks #352
Conversation
Current benchmark values: [info] Benchmark Mode Cnt Score Error Units [info] StringInBenchmarks.linearMatchIn avgt 3 77.603 ± 0.529 ns/op [info] StringInBenchmarks.radixMatchIn avgt 3 59.675 ± 2.126 ns/op
Codecov Report
@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 96.56% 96.71% +0.15%
==========================================
Files 9 9
Lines 1049 1128 +79
Branches 94 99 +5
==========================================
+ Hits 1013 1091 +78
- Misses 36 37 +1
Continue to review full report at Codecov.
|
results: [info] Benchmark (test) Mode Cnt Score Error Units [info] StringInBenchmarks.linearMatchIn foo avgt 3 77.718 ± 3.173 ns/op [info] StringInBenchmarks.linearMatchIn broad avgt 3 92973.234 ± 2156.151 ns/op [info] StringInBenchmarks.oneOfParse foo avgt 3 91.240 ± 4.854 ns/op [info] StringInBenchmarks.oneOfParse broad avgt 3 806.046 ± 37.539 ns/op [info] StringInBenchmarks.radixMatchIn foo avgt 3 59.750 ± 1.315 ns/op [info] StringInBenchmarks.radixMatchIn broad avgt 3 769.000 ± 25.609 ns/op [info] StringInBenchmarks.stringInParse foo avgt 3 91.551 ± 0.600 ns/op [info] StringInBenchmarks.stringInParse broad avgt 3 809.561 ± 54.042 ns/op
I added a benchmark that is close to ideal for the radix tree to have a kind of upper bound comparison. In that example this PR gives performance about 27% faster than main (and 90x faster than a naive linear search). So, I think it is fair to say the worst case performance of the new code is slightly worse than a linear search (17% comparing the favorable linear search benchmark with the tree). Note, linear search can be very good with only a few alternatives. But when there are many alternatives, the tree can be far, far better.
|
@regadas if you have time, I'd love your review (or anyone). I think this is ready. |
Hi, @johnynek this one slipped under my radar. I'll take a look at it tmr. |
Thank you! I know this OSS stuff can be a grind. Don't worry at all. I appreciate your help. |
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.
This looks great! Sorry for the delay @johnynek
def matchAt(str: String, off: Int): Int = | ||
matchAtOrNull(str, off) match { | ||
case null => -1 | ||
case nonNull => nonNull.length |
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.
Actually I think this is wrong. It has to be offset + NonNull.length, or the comment is wrong. Maybe we should just fix the comment, but also the Parser is assuming it returns the new offset not the length.
I'll send a PR with a failing test and then fix.
I think this is happening because we are only testing at offset 0.
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.
Nice catch!
Being offset + nonNull.length
makes more sense and the consistency with matchAtOrNull
reflects it (which needs a fix).
These make more sense now
assert(len <= targ.length, s"len = $len, off = $off")
assertEquals(left, targ.substring(off, len), s"len = $len, left = $left")
part of #350
This may be marginally faster, but the main value it adds is that RadixNode can now return the matching string without allocating. Also, I think the implementation is somewhat simpler.
I expect this should be faster if the set of strings gets larger (the benchmarks only have a few different prefixes. This uses a hash based approach so it is always O(1) to check a character (not log(N) worst case).