Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

optimize uri parsing to reduce StringBuilder allocations #330

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

fwbrasil
Copy link
Contributor

Problem

The uri parsing has a high allocation rate of StringBuilder objects in ArrayView.mkStringOpt. The parsing happens on each incoming request and the implementation of mkStringOpt uses a large StringBuilder initialized with 128 chars. The actual strings are typically much smaller representing a single path element.

Solution

I've debugged the code and noticed that the vast majority of calls have a valueTs with a single path element. I've added a special case for it so mkStringOpt can be avoided.

Notes

  • This is part of Reduce allocation rate tapir#3552.
  • It's probably better to avoid the custom initial StringBuilder capacity in mkStringOpt since the strings seem generally small and growing the buffer is typically not too expensive. I can follow up on that if you like.
  • Another alternative is optimizing the code that uses mkStringOpt.

@adamw
Copy link
Member

adamw commented Feb 29, 2024

@kciesielski that's exactly the scenario we've been talking about today: Uri parsing that has a single string component.

as a follow-up, I think it might be good to (create an issue, as a start) to take a look at the interpolation cases, where we have a couple of string components - allocating the large StringBuilder is likely to be a bottleneck there as well

@@ -731,6 +731,14 @@ object UriInterpolator {
case Singleton(ExpressionToken(s: Array[_])) =>
b ++= s.flatMap(anyToStringOpt)
doToSeq(tailTs)
case valueTs if(valueTs.size == 1) =>
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment here why the optimization is in place? with the issue number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! I've just added it

@kciesielski
Copy link
Member

Nice, it could help to resolve expensive Uri parsing in softwaremill/tapir#3547 and softwaremill/tapir#3550. I'll build this locally and see if it reduces overhead in the performance tests.

@fwbrasil fwbrasil force-pushed the opt-uri-token-decoding branch from 7222152 to abfc7fe Compare March 4, 2024 17:55
@fwbrasil fwbrasil force-pushed the opt-uri-token-decoding branch from abfc7fe to 2aae990 Compare March 4, 2024 17:55
@kciesielski
Copy link
Member

kciesielski commented Mar 5, 2024

Update: An improvement is visible when profiling performance tests with the DefaultServerLog interceptor enabled.
Frame: NettyServerRequest.uri
Without optimization: CPU: 6.81% samples / 12.44% allocations
With optimization: CPU 5.45% samples / 11.68% allocations

@adamw adamw merged commit 45466f4 into softwaremill:master Mar 5, 2024
4 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants