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

Only allocate a new (random) trace ID when trace ID parse fails rathe… #161

Merged
merged 3 commits into from
Jun 8, 2020

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Jun 8, 2020

…r than all the time and prepare for immutable.

Noticed that we always allocate a fresh TraceID even when parsing one successfully. We should only do so in the case of a failure, since generating a random is very expensive. Most parses are successful. Around 4x speedup.

Also cleaned up delimiter parsing using @shengxil's pattern and prepared to make this an immutable class by deprecating setters / constructors and creating factories.

After

Benchmark                                                      Mode     Cnt     Score     Error   Units
TraceHeaderBenchmark.parse                                   sample  556594     0.555 ±   0.025   us/op
TraceHeaderBenchmark.parse:parse·p0.00                       sample             0.339             us/op
TraceHeaderBenchmark.parse:parse·p0.50                       sample             0.405             us/op
TraceHeaderBenchmark.parse:parse·p0.90                       sample             0.542             us/op
TraceHeaderBenchmark.parse:parse·p0.95                       sample             0.746             us/op
TraceHeaderBenchmark.parse:parse·p0.99                       sample             1.914             us/op
TraceHeaderBenchmark.parse:parse·p0.999                      sample            19.232             us/op
TraceHeaderBenchmark.parse:parse·p0.9999                     sample            52.097             us/op
TraceHeaderBenchmark.parse:parse·p1.00                       sample          1740.800             us/op
TraceHeaderBenchmark.parse:·gc.alloc.rate                    sample      15  1322.606 ±  78.021  MB/sec
TraceHeaderBenchmark.parse:·gc.alloc.rate.norm               sample      15   888.236 ±   0.024    B/op
TraceHeaderBenchmark.parse:·gc.churn.G1_Eden_Space           sample      15  1331.803 ± 119.214  MB/sec
TraceHeaderBenchmark.parse:·gc.churn.G1_Eden_Space.norm      sample      15   894.092 ±  54.121    B/op
TraceHeaderBenchmark.parse:·gc.churn.G1_Survivor_Space       sample      15     0.093 ±   0.014  MB/sec
TraceHeaderBenchmark.parse:·gc.churn.G1_Survivor_Space.norm  sample      15     0.062 ±   0.011    B/op
TraceHeaderBenchmark.parse:·gc.count                         sample      15   145.000            counts
TraceHeaderBenchmark.parse:·gc.time                          sample      15   128.000                ms

Before

Benchmark                                                      Mode     Cnt     Score    Error   Units
TraceHeaderBenchmark.parse                                   sample  437664     2.024 ±  0.209   us/op
TraceHeaderBenchmark.parse:parse·p0.00                       sample             0.498            us/op
TraceHeaderBenchmark.parse:parse·p0.50                       sample             0.800            us/op
TraceHeaderBenchmark.parse:parse·p0.90                       sample             0.934            us/op
TraceHeaderBenchmark.parse:parse·p0.95                       sample             1.170            us/op
TraceHeaderBenchmark.parse:parse·p0.99                       sample             4.496            us/op
TraceHeaderBenchmark.parse:parse·p0.999                      sample            40.021            us/op
TraceHeaderBenchmark.parse:parse·p0.9999                     sample          1761.280            us/op
TraceHeaderBenchmark.parse:parse·p1.00                       sample          3551.232            us/op
TraceHeaderBenchmark.parse:·gc.alloc.rate                    sample      15   627.784 ± 53.949  MB/sec
TraceHeaderBenchmark.parse:·gc.alloc.rate.norm               sample      15  1072.556 ±  0.061    B/op
TraceHeaderBenchmark.parse:·gc.churn.G1_Eden_Space           sample      15   631.644 ± 86.257  MB/sec
TraceHeaderBenchmark.parse:·gc.churn.G1_Eden_Space.norm      sample      15  1076.679 ± 92.875    B/op
TraceHeaderBenchmark.parse:·gc.churn.G1_Survivor_Space       sample      15     0.100 ±  0.017  MB/sec
TraceHeaderBenchmark.parse:·gc.churn.G1_Survivor_Space.norm  sample      15     0.172 ±  0.030    B/op
TraceHeaderBenchmark.parse:·gc.count                         sample      15    94.000           counts
TraceHeaderBenchmark.parse:·gc.time                          sample      15    95.000               ms

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Anuraag Agrawal added 2 commits June 8, 2020 10:38
@anuraaga
Copy link
Contributor Author

anuraaga commented Jun 8, 2020

By the way, I find the order of methods to look weird when using factories. Filed an issue in aws sdk repo so we can consider the issue consistently aws/aws-sdk-java-v2#1880

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

LGTM, much more readable! Should we make the TraceID class fully immutable in 3.x?

@@ -22,54 +22,77 @@

public class TraceID {

private static final int TRACE_ID_LENGTH = 35;
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: increase by 1 in ~85 years :D

@willarmiros
Copy link
Contributor

Also, do you think you could include the benchmarks you're running in the Java SDK benchmark package for future reference?

@anuraaga
Copy link
Contributor Author

anuraaga commented Jun 8, 2020

@anuraaga anuraaga merged commit 7e8c33c into aws:master Jun 8, 2020
@anuraaga
Copy link
Contributor Author

anuraaga commented Jun 8, 2020

Yup in 3.x will make this immutable, deprecated the setters in preparation.

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