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

Optimize UserAgents #868

Merged
merged 4 commits into from
Jul 21, 2022
Merged

Optimize UserAgents #868

merged 4 commits into from
Jul 21, 2022

Conversation

schlosna
Copy link
Contributor

Before this PR

Similar to #864 , JFRs showing significant allocations and CPU consumed due to com.palantir.conjure.java.api.config.service.UserAgents#isValidName and com.palantir.conjure.java.api.config.service.UserAgents#format during Dialogue request processing

image
image

After this PR

Some initial benchmark results on 16 core Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz where regex is the previous implementation and parser is our hand rolled non-allocating version.

JDK 11.0.13, OpenJDK 64-Bit Server VM, 11.0.13+8-LTS
Benchmark                                                      (name)  Mode  Cnt    Score      Error   Units
UserAgentsBenchmark.parser                                    service  avgt    3    0.021 ±    0.002   us/op
UserAgentsBenchmark.parser:·gc.alloc.rate.norm                service  avgt    3   ≈ 10⁻⁵               B/op
UserAgentsBenchmark.parser                     valid-service-name-123  avgt    3    0.073 ±    0.005   us/op
UserAgentsBenchmark.parser:·gc.alloc.rate.norm valid-service-name-123  avgt    3   ≈ 10⁻⁵               B/op
UserAgentsBenchmark.regex                                     service  avgt    3    0.092 ±    0.055   us/op
UserAgentsBenchmark.regex:·gc.alloc.rate.norm                 service  avgt    3  128.000 ±    0.001    B/op
UserAgentsBenchmark.regex                      valid-service-name-123  avgt    3    0.299 ±    0.005   us/op
UserAgentsBenchmark.regex:·gc.alloc.rate.norm  valid-service-name-123  avgt    3  200.000 ±    0.001    B/op
JDK 17.0.4, OpenJDK 64-Bit Server VM, 17.0.4+8-LTS
Benchmark                                                      (name)  Mode  Cnt     Score      Error   Units
UserAgentsBenchmark.parser                                    service  avgt    3     0.015 ±    0.002   us/op
UserAgentsBenchmark.parser:·gc.alloc.rate.norm                service  avgt    3    ≈ 10⁻⁵               B/op
UserAgentsBenchmark.parser                     valid-service-name-123  avgt    3     0.030 ±    0.001   us/op
UserAgentsBenchmark.parser:·gc.alloc.rate.norm valid-service-name-123  avgt    3    ≈ 10⁻⁵               B/op
UserAgentsBenchmark.regex                                     service  avgt    3     0.078 ±    0.021   us/op
UserAgentsBenchmark.regex:·gc.alloc.rate.norm                 service  avgt    3   128.008 ±    0.043    B/op
UserAgentsBenchmark.regex                      valid-service-name-123  avgt    3     0.251 ±    0.017   us/op
UserAgentsBenchmark.regex:·gc.alloc.rate.norm  valid-service-name-123  avgt    3   200.013 ±    0.001    B/op

==COMMIT_MSG==
Optimize UserAgents

Optimize UserAgents.isValidName to use a hand-rolled implementation to validate user agent name to avoid
allocation and regex overhead.

Increase default UserAgents.format string builder buffer size and ensure capacity as we append user agents.
==COMMIT_MSG==

Possible downsides?

Complexity of maintaining is higher than simple regex

Use a hand-rolled implementation to validate user agent name to avoid
allocation and regex overhead.
@changelog-app
Copy link

changelog-app bot commented Jul 21, 2022

Generate changelog in changelog/@unreleased

Type
See change types. Select one:

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Optimize UserAgents

Optimize UserAgents.isValidName to use a hand-rolled implementation to validate user agent name to avoid
allocation and regex overhead.

Increase default UserAgents.format string builder buffer size and ensure capacity as we append user agents.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@iamdanfox iamdanfox left a comment

Choose a reason for hiding this comment

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

Optimized code remains very readable. I'm always surprised at how expensive regexes are in java!

@schlosna
Copy link
Contributor Author

Tweaked the implementation a bit more exactly follow regex and improved a bit more:

JDK 17.0.4, OpenJDK 64-Bit Server VM, 17.0.4+8-LTS
Benchmark                                                      (name)  Mode  Cnt     Score     Error   Units
UserAgentsBenchmark.parser                                    service  avgt    3     0.008 ±   0.001   us/op
UserAgentsBenchmark.parser:·gc.alloc.rate.norm                service  avgt    3    ≈ 10⁻⁵              B/op
UserAgentsBenchmark.parser                     valid-service-name-123  avgt    3     0.018 ±   0.002   us/op
UserAgentsBenchmark.parser:·gc.alloc.rate.norm valid-service-name-123  avgt    3    ≈ 10⁻⁵              B/op
UserAgentsBenchmark.regex                                     service  avgt    3     0.077 ±   0.006   us/op
UserAgentsBenchmark.regex:·gc.alloc.rate.norm                 service  avgt    3   128.009 ±   0.028    B/op
UserAgentsBenchmark.regex                      valid-service-name-123  avgt    3     0.251 ±   0.148   us/op
UserAgentsBenchmark.regex:·gc.alloc.rate.norm  valid-service-name-123  avgt    3   200.013 ±   0.008    B/op

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@bulldozer-bot bulldozer-bot bot merged commit f43240f into develop Jul 21, 2022
@bulldozer-bot bulldozer-bot bot deleted the ds/user-agents branch July 21, 2022 22:16
@svc-autorelease
Copy link
Collaborator

Released 2.27.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants