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

Avoid unnecessary checks when calling readCodePointValue on Buffer #343

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

fzhinkin
Copy link
Collaborator

Improves readCodePointValue performance by ~10% when the receiver is Buffer.

Addresses a particular issue uncovered in Kotlin/kotlinx.serialization#2707.

In general, similar issues will be tracked by #342.

Benchmarking results w/ jdk21 on macOS host w/ AS M2 Pro CPU:

  • before:
Benchmark                                                (encoding)  (minGap)   Mode  Cnt          Score         Error  Units
Utf8CodePointsBenchmark.benchmark                             ascii       128  thrpt    5  140373273.339 ±  694216.983  ops/s
Utf8CodePointsBenchmark.benchmark                              utf8       128  thrpt    5   87815553.220 ± 1128003.103  ops/s
Utf8CodePointsBenchmark.benchmark                            sparse       128  thrpt    5  127022106.163 ± 2328819.676  ops/s
Utf8CodePointsBenchmark.benchmark                            2bytes       128  thrpt    5  114376689.982 ± 1040653.731  ops/s
Utf8CodePointsBenchmark.benchmark                            3bytes       128  thrpt    5   97996115.414 ±  943350.071  ops/s
Utf8CodePointsBenchmark.benchmark                            4bytes       128  thrpt    5   90219791.071 ± 1057392.878  ops/s
Utf8CodePointsBenchmark.benchmark                               bad       128  thrpt    5  129320612.342 ± 1542655.859  ops/s
  • after:
Benchmark                                                (encoding)  (minGap)   Mode  Cnt          Score         Error  Units
Utf8CodePointsBenchmark.benchmark                             ascii       128  thrpt    5  141722288.764 ± 1297310.006  ops/s
Utf8CodePointsBenchmark.benchmark                              utf8       128  thrpt    5   96539014.741 ±  397371.538  ops/s
Utf8CodePointsBenchmark.benchmark                            sparse       128  thrpt    5  136350281.578 ± 9488253.150  ops/s
Utf8CodePointsBenchmark.benchmark                            2bytes       128  thrpt    5  126114440.572 ± 2201911.847  ops/s
Utf8CodePointsBenchmark.benchmark                            3bytes       128  thrpt    5  110880125.804 ± 1953517.672  ops/s
Utf8CodePointsBenchmark.benchmark                            4bytes       128  thrpt    5  101067180.434 ± 1928563.268  ops/s
Utf8CodePointsBenchmark.benchmark                               bad       128  thrpt    5  146300685.623 ± 2768473.597  ops/s

@fzhinkin fzhinkin merged commit e0d5fa9 into develop Jun 12, 2024
1 check passed
@fzhinkin fzhinkin deleted the improve-read-cp-value-performance branch June 12, 2024 10:55
@@ -260,6 +260,9 @@ public fun Source.readString(byteCount: Long): String {
*/
@OptIn(InternalIoApi::class)
public fun Source.readCodePointValue(): Int {
if (this is Buffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@fzhinkin question, would this be a valid solution to avoid an is check?

public fun Buffer.readCodePointValue(): Int {
  return commonReadUtf8CodePoint()
}

public fun Source.readCodePointValue(): Int { 
  ... 
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it wouldn't, as extension functions are resolved statically based on the declared receiver's type.

So if there's a code like

val src: Source = Buffer().apply { ... }
println(src.readCodePointValue())

readCodePointValue will be resolved to Source.readCodePointValue and an extension defined on Buffer won't help.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fzhinkin thanks! If we have the additional extension function tho, it will avoid the check when the type is Buffer, which I think is the most common case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will avoid the check when the type is Buffer, which I think is the most common case.

Indeed, at least for Okio, the corresponding function is invoked on Buffer more frequently, compared to BufferedSource.

I don't really like the idea of adding an extra extension function, as performance benefits should be neglectingly small. On the other hand, there will be some benefits, so I consider adding the function later in the course of #342.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I think the best approach would be to do the same thing you did for unsafe operations, throw in a quick kotlinx-benchmark. Would be interesting to document all of these cases for JS.

For K/JS here we are lucky because Buffer is a class, and the is check will be compiled to a native instanceof. When the check is done on an interface, there is a lot more work involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants