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

Makes entry splitter work with CharSequence #1284

Merged
merged 6 commits into from
Dec 24, 2020
Merged

Makes entry splitter work with CharSequence #1284

merged 6 commits into from
Dec 24, 2020

Conversation

codefromthecrypt
Copy link
Member

This allows us to split and parse CharSequence instead of String, which
reduces allocations when parsing W3C tracestate.

This adds a utility CharSequences to help with tasks.

This allows us to split and parse CharSequence instead of String, which
reduces allocations when parsing W3C tracestate.

This adds a utility `CharSequences` to help with tasks.
codefromthecrypt pushed a commit to openzipkin-contrib/brave-propagation-w3c that referenced this pull request Dec 23, 2020
This reworks TraceContextExtractor using utilities recently added to brave:
openzipkin/brave#1284

The main idea is that this allows us to split tracestate without allocating
any strings. The current implementation remains incomplete, but this phase
sets the stage for completion.
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding any jmh? String is workhorse of Java so it'd be nice to confirm it doesn't have any magic defeating CharSequence even at the expense of some alloc for a realish type of usage.

@codefromthecrypt
Copy link
Member Author

will do on adding JMH

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Dec 24, 2020

Ok rejigged after noticing String's behaviour on subsequence. Here are the bench results in my laptop for somewhat realistic data. The only place this is of real use is when splitting the middle of a string (ex to own an entry). This can happen at least in w3c tracestate and also secondary sampling.

$ egrep '(^Bench|p0.999 |gc.alloc.rate.norm)' /tmp/foo
Benchmark                                                                                                          Mode     Cnt     Score     Error   Units
CharSequencesBenchmarks.regionMatches_hit:regionMatches_hit·p0.999                                               sample             0.245             us/op
CharSequencesBenchmarks.regionMatches_hit:·gc.alloc.rate.norm                                                    sample      15     0.005 ±   0.001    B/op
CharSequencesBenchmarks.regionMatches_miss:regionMatches_miss·p0.999                                             sample             0.162             us/op
CharSequencesBenchmarks.regionMatches_miss:·gc.alloc.rate.norm                                                   sample      15     0.001 ±   0.001    B/op
CharSequencesBenchmarks.withoutSubSequence_middle_max:withoutSubSequence_middle_max·p0.999                       sample             0.674             us/op
CharSequencesBenchmarks.withoutSubSequence_middle_max:·gc.alloc.rate.norm                                        sample      15    40.005 ±   0.001    B/op
CharSequencesBenchmarks.withoutSubSequence_middle_max_string:withoutSubSequence_middle_max_string·p0.999         sample            10.880             us/op
CharSequencesBenchmarks.withoutSubSequence_middle_max_string:·gc.alloc.rate.norm                                 sample      15  6040.687 ±   0.033    B/op
CharSequencesBenchmarks.withoutSubSequence_middle_normal:withoutSubSequence_middle_normal·p0.999                 sample             1.508             us/op
CharSequencesBenchmarks.withoutSubSequence_middle_normal:·gc.alloc.rate.norm                                     sample      15    40.005 ±   0.001    B/op
CharSequencesBenchmarks.withoutSubSequence_middle_normal_string:withoutSubSequence_middle_normal_string·p0.999   sample             1.710             us/op
CharSequencesBenchmarks.withoutSubSequence_middle_normal_string:·gc.alloc.rate.norm                              sample      15   424.042 ±   0.002    B/op

@codefromthecrypt
Copy link
Member Author

@jeqo one thing I mentioned to rag, but not you yet. The trace-context format is a performance bomb for messaging at the point anyone uses tracestate header. One thing this does is insulate us, such that if receiving 1000 messages doesn't mean allocating some factor N strings, some of which may never be used if the messages are filtered out.

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Dec 24, 2020

went ahead and added bench missing for entry splitter. notably it shows nothing is allocated.

Benchmark                                                                       Mode     Cnt      Score     Error   Units
EntrySplitterBenchmarks.parse_max:parse_max·p0.999                            sample             15.578             us/op
EntrySplitterBenchmarks.parse_max:·gc.alloc.rate.norm                         sample      15      0.458 ±   0.056    B/op
EntrySplitterBenchmarks.parse_max_string:parse_max_string·p0.999              sample             23.178             us/op
EntrySplitterBenchmarks.parse_max_string:·gc.alloc.rate.norm                  sample      15  11570.455 ±   0.124    B/op
EntrySplitterBenchmarks.parse_normal:parse_normal·p0.999                      sample             10.272             us/op
EntrySplitterBenchmarks.parse_normal:·gc.alloc.rate.norm                      sample      15      0.033 ±   0.004    B/op
EntrySplitterBenchmarks.parse_normal_string:parse_normal_string·p0.999        sample             11.072             us/op
EntrySplitterBenchmarks.parse_normal_string:·gc.alloc.rate.norm               sample      15   1048.233 ±   0.011    B/op

@codefromthecrypt codefromthecrypt merged commit d47e861 into master Dec 24, 2020
codefromthecrypt pushed a commit to openzipkin-contrib/brave-propagation-w3c that referenced this pull request Dec 24, 2020
This reworks TraceContextExtractor using utilities recently added to brave:
openzipkin/brave#1284

The main idea is that this allows us to split tracestate without allocating
any strings. The current implementation remains incomplete, but this phase
sets the stage for completion.
codefromthecrypt pushed a commit to openzipkin-contrib/brave-propagation-w3c that referenced this pull request Dec 24, 2020
* Overhauls TraceContextExtractor

This reworks TraceContextExtractor using utilities recently added to brave:
openzipkin/brave#1284

The main idea is that this allows us to split tracestate without allocating
any strings. The current implementation remains incomplete, but this phase
sets the stage for completion.

* foo
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.

2 participants