[PR 4/5] Rewrite existing extensions using unsafe API #337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fzhinkin
force-pushed
the
bulk-api-part-2
branch
from
0a8eff3
to
94e8786
Compare
fzhinkin
force-pushed
the
bulk-api-part-3
branch
from
1855228
to
0a163a5
Compare
shanshin
reviewed
Jun 21, 2024
core/common/src/Buffers.kt
Outdated
| @@ -22,7 +22,9 @@ public fun Buffer.snapshot(): ByteString { | |||
| var curr = head | |||
| do { | |||
| check(curr != null) { "Current segment is null" } | |||
| append(curr.data, curr.pos, curr.limit) | |||
| for (idx in 0 until curr.size) { | |||
There was a problem hiding this comment.
Is it not degrading performance?
It seems to me that writing byte-by-byte will be very slow due to the large number of checks and frequent copyInto calls
There was a problem hiding this comment.
Thanks, I'll reimplement it. We don't track Buffer.snapshot performance, so it didn't ring the bell once I plunked a naive implementation.
fzhinkin
force-pushed
the
bulk-api-part-2
branch
from
94e8786
to
2f3b57b
Compare
Sure, I'll publish results. |
|
@shanshin, so, regarding the performance. Of course, adding a level of abstraction over a byte array won't make it faster compared to direct access to the byte array. And that's true for older JDK versions. Here, for instance, benchmarking results collected using JDK11:
tl;dr: on average, everything slowed down and UTF8-processing became 15-50% slower: Summary table for Utf8StringBenchmark
However, more up-to-date runtimes are capable of handling this new level of abstraction, resulting in the same or even better performance compared to what could be achieved with the code from the master branch.
tl;dr: a few benchmarks show some slowdown, but in general, performance either improved or remained at the same level. Summary table for Utf8StringBenchmark
On the Android, results are somewhat similar.
Summary table for Utf8StringBenchmark
However, when R8 kicks in, results start looking similar to what was observed with recent JDK versions:
Summary table for Utf8StringBenchmark
To summarize: it's definitely hard to beat a code reading/writing directory to/from a byte array by wrapping these accesses into interface calls. But modern optimizing compiler are good at it and allow preserving the same performance level. |
|
@fzhinkin what about the perf impact for non-JVM platforms, like Native, or they aren't impacted at all? |
|
@lppedd, on Native, marginally, there's a small slowdown when it comes to UTF8 processing, but overall, things look good:
|
|
@fzhinkin thanks! Do you save previous benchmark results somewhere? |
|
@lppedd, no, I don't, but it's worth saving. Thanks for the suggestion! |
shanshin
approved these changes
Aug 13, 2024
fzhinkin
force-pushed
the
bulk-api-part-3
branch
from
23aaaab
to
0a86a46
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reimplements existing extensions to use unsafe API introduced in #334 and #336.
In the next patch, Segment's
datafield will be encapsulated.Related to #135