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

Improve RadixNode add a few more benchmarks #352

Merged
merged 11 commits into from
Jan 27, 2022
Merged

Conversation

johnynek
Copy link
Collaborator

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).

Before:

[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

This PR:

[info] Benchmark                         Mode  Cnt   Score   Error  Units
[info] StringInBenchmarks.linearMatchIn  avgt    3  77.427 ± 4.083  ns/op
[info] StringInBenchmarks.radixMatchIn   avgt    3  59.067 ± 4.797  ns/op

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-commenter
Copy link

codecov-commenter commented Jan 16, 2022

Codecov Report

Merging #352 (d79bcfa) into main (3acc967) will increase coverage by 0.15%.
The diff coverage is 97.33%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...e/shared/src/main/scala/cats/parse/RadixNode.scala 95.31% <95.08%> (-4.69%) ⬇️
core/shared/src/main/scala/cats/parse/Parser.scala 96.69% <98.87%> (+0.27%) ⬆️
...shared/src/main/scala/cats/parse/Accumulator.scala 100.00% <0.00%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b524da...d79bcfa. Read the comment docs.

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
@johnynek
Copy link
Collaborator Author

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.

// With new benchmarks:
[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

// new benchmarks on main
[info] Benchmark                         (test)  Mode  Cnt      Score      Error  Units
[info] StringInBenchmarks.linearMatchIn     foo  avgt    3     78.476 ±    2.503  ns/op
[info] StringInBenchmarks.linearMatchIn   broad  avgt    3  93335.231 ± 2911.905  ns/op
[info] StringInBenchmarks.oneOfParse        foo  avgt    3     96.497 ±    1.285  ns/op
[info] StringInBenchmarks.oneOfParse      broad  avgt    3   1107.582 ±    1.940  ns/op
[info] StringInBenchmarks.stringInParse     foo  avgt    3     96.489 ±    0.397  ns/op
[info] StringInBenchmarks.stringInParse   broad  avgt    3   1106.763 ±    3.840  ns/op

@johnynek
Copy link
Collaborator Author

@regadas if you have time, I'd love your review (or anyone).

I think this is ready.

@johnynek johnynek requested a review from regadas January 18, 2022 18:15
@regadas
Copy link
Collaborator

regadas commented Jan 25, 2022

Hi, @johnynek this one slipped under my radar. I'll take a look at it tmr.

@johnynek
Copy link
Collaborator Author

Thank you! I know this OSS stuff can be a grind. Don't worry at all. I appreciate your help.

Copy link
Collaborator

@regadas regadas left a 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

@regadas regadas merged commit 3dda8ab into main Jan 27, 2022
@regadas regadas deleted the oscar/improve_radix_bench branch January 27, 2022 07:56
def matchAt(str: String, off: Int): Int =
matchAtOrNull(str, off) match {
case null => -1
case nonNull => nonNull.length
Copy link
Collaborator Author

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.

Copy link
Collaborator

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")

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