-
Notifications
You must be signed in to change notification settings - Fork 261
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
Avoid array copy on LocalResponse.getBodyAsString #93
Conversation
return body; | ||
} | ||
|
||
@Override | ||
public String getBodyAsString() throws IOException { | ||
return output.toString(getCharset().name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the default implementation doing pretty much the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default one uses getBody() which uses output.getByteArray() which makes copy.
So if codepath uses getBodyAsString, this setup avoids array copying by constructing string directly from output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default one uses getBody() which uses output.getByteArray() which makes copy.
So if codepath uses getBodyAsString, this setup avoids array copying by constructing string directly from output.
But getBody
doesn't do the copying, withBody
does and that should only happen once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this change withBody
does not do copy anymore.
👍 |
👍 |
1 similar comment
+1 |
+1 |
@AlexanderYastrebov Can you approve + merge? |
Make withBody noop and getBody create bytearray lazily
Use output's buffer directly in getBodyAsString