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

Content.Sink.write(sink, last, utf8Content, callback) could become faster #12469

Closed
gnikolaidis opened this issue Nov 3, 2024 · 8 comments · Fixed by #12475
Closed

Content.Sink.write(sink, last, utf8Content, callback) could become faster #12469

gnikolaidis opened this issue Nov 3, 2024 · 8 comments · Fixed by #12475
Assignees

Comments

@gnikolaidis
Copy link

Jetty version(s)
Jetty 12.0.x

Enhancement Description
Looking at the source code, I see that the method body of Content.Sink.write(sink, last, utf8Content, callback) is using StandardCharsets.UTF_8.encode(utf8Content) to create the UTF-8 ByteBuffer.

I believe that ByteBuffer.wrap(utf8Content.getBytes(StandardCharsets.UTF_8)) would be quite faster, as in recent Java versions (which use UTF8 internally) it is implemented as a simple System.arrayCopy().

@lorban
Copy link
Contributor

lorban commented Nov 4, 2024

You are right, a quick JMH benchmark clearly shows that wrap/getBytes is consistenly faster than encoding:

Fully ASCII string
 JDK 22
  Utf8Benchmark.testEncode        thrpt   10   8642774.694 ± 194220.398  ops/s
  Utf8Benchmark.testWrapGetBytes  thrpt   10  52008364.510 ± 271773.779  ops/s
 JDK 17
  Utf8Benchmark.testEncode        thrpt   10   7355221.908 ± 396420.981  ops/s
  Utf8Benchmark.testWrapGetBytes  thrpt   10  52114741.560 ± 124515.129  ops/s

French string
 JDK 22
  Utf8Benchmark.testEncode        thrpt   10   4858458.628 ± 269877.435  ops/s
  Utf8Benchmark.testWrapGetBytes  thrpt   10  17264867.586 ± 227175.842  ops/s
 JDK 17
  Utf8Benchmark.testEncode        thrpt   10   4346969.813 ± 213333.374  ops/s
  Utf8Benchmark.testWrapGetBytes  thrpt   10  16451271.214 ± 125170.260  ops/s

Japanese string
 JDK 22
  Utf8Benchmark.testEncode        thrpt   10  2819575.012 ± 142874.446  ops/s
  Utf8Benchmark.testWrapGetBytes  thrpt   10  7802310.700 ±  74366.173  ops/s
 JDK 17
  Utf8Benchmark.testEncode        thrpt   10  2670866.437 ± 155794.533  ops/s
  Utf8Benchmark.testWrapGetBytes  thrpt   10  8240865.962 ±  70066.445  ops/s

so I'm going to create a pull request to submit this enhancement.

Thanks for the suggestion!

@lorban lorban moved this to 🏗 In progress in Jetty 12.0.16 FROZEN Nov 4, 2024
@joakime
Copy link
Contributor

joakime commented Nov 4, 2024

@lorban what's the GC difference like?

lorban added a commit that referenced this issue Nov 4, 2024
Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor

lorban commented Nov 4, 2024

@joakime Interesting question. JMH's GCProfiler reports that the encode benchmark generates about 2 to 10 GB/s of garbage while wrap/getBytes generates about 25 GB/s... which is probably about the max memory bandwidth of my machine.

Still, GC'ing seems to be faster than encoding.

@sbordet
Copy link
Contributor

sbordet commented Nov 4, 2024

How is this not a JDK bug?

@gnikolaidis
Copy link
Author

My guess as to why it not a JDK bug is that Charset.encode(String) is very explicit that it must produce the same result as Charset.encode(CharBuffer), which in turn - quoting the Javadoc - always replaces malformed-input and unmappable-character sequences with charset's default replacement string - which may or may not be the case with JDK's UTF-8 encoding.

@gnikolaidis
Copy link
Author

@joakime Interesting question. JMH's GCProfiler reports that the encode benchmark generates about 2 to 10 GB/s of garbage while wrap/getBytes generates about 25 GB/s... which is probably about the max memory bandwidth of my machine.

Still, GC'ing seems to be faster than encoding.

These results are probably just an artifact of the higher overall throughput.

@joakime
Copy link
Contributor

joakime commented Nov 4, 2024

My guess as to why it not a JDK bug is that Charset.encode(String) is very explicit that it must produce the same result as Charset.encode(CharBuffer), which in turn - quoting the Javadoc - always replaces malformed-input and unmappable-character sequences with charset's default replacement string - which may or may not be the case with JDK's UTF-8 encoding.

Error handling / Bad input handling is an important aspect of this.

@lorban what happens if the new technique encounters bad/malformed input?
Will it throw an exception? or will it replace characters with the UTF-8 replacement character?

lorban added a commit that referenced this issue Nov 5, 2024
@lorban
Copy link
Contributor

lorban commented Nov 5, 2024

@gnikolaidis I've modified my benchmark to make it more apparent that when the given string is purely ASCII, the wrap/getBytes throughput bottlenecks on memory bandwidth, so it's purely a memcopy/GC benchmark in that case. In all other cases, some encoding logic has to run and it's more complex to compare.

@joakime since the source is a String object, wouldn't the invalid bytes be returned as-is in the returned byte array?

lorban added a commit that referenced this issue Nov 5, 2024
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Nov 6, 2024
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Nov 6, 2024
lorban added a commit that referenced this issue Nov 6, 2024
Signed-off-by: Ludovic Orban <[email protected]>
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.16 FROZEN Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants