-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use faster UTF8 encoding in Content.write()
#12475
Conversation
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
tests/jetty-jmh/src/main/java/org/eclipse/jetty/io/jmh/Utf8Benchmark.java
Outdated
Show resolved
Hide resolved
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.
Please confirm that the behavior when dealing with bad input is the same with the new code vs the old code.
Example:
A bad / Malformed input String that would trigger a fault in the conversion.
If the old code throws an exception, this new one should too.
If the old code used replacement characters, this new one should too.
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@joakime is it possible to create a |
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
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.
It is difficult to create a String that CharsetDecoder.encode(String) would fail on.
I found one other place where we use |
For future reference, here is the benchmark's report:
|
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.
Is there anything that can be done to improve org.eclipse.jetty.ee11.servlet.HttpOutput#print(java.lang.String, boolean)?
@gregw We could theoretically replace all that with a much simpler |
If the behavior of the API to the users is maintained, then I'm in favor of the change. |
I'm going to give |
Fixes #12469