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

KTOR-7941 Fix performance on readUtfLineTo #4537

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Conversation

bjhham
Copy link
Contributor

@bjhham bjhham commented Dec 9, 2024

Subsystem
Core, I/O

Motivation
KTOR-7941 A Performance issue reading with ByteReadChannel.readUTF8LineTo request body

Solution
When reading input that is missing either CR or LF characters, the current implementation will read the full buffer for every line, which leads to a O(n^2) performance, which is very bad for this kind of operation when there are many newlines with only \n for example.

@bjhham bjhham requested a review from e5l December 9, 2024 14:44
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Could you add a benchmark before merging?

while (!isClosedForRead) {
while (!readBuffer.exhausted()) {
when (val b = readBuffer.readByte()) {
CR -> {
Copy link
Member

Choose a reason for hiding this comment

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

I was assuming indexOf is working faster than manual readByte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's all buffered anyway so it seems to be roughly the same implementation.

Oh! I just realized why it's so slow! It's because if you have an input with no \r characters, then it would be O(n^2) for the previous implementation, because it searches to the end of the content every time!

@bjhham
Copy link
Contributor Author

bjhham commented Dec 11, 2024

You mean adding a benchmark to ktor-benchmarks/io-benchmarks right?

@bjhham bjhham force-pushed the bjhham/readLinePerfFix branch from 63e2e21 to ef7c54c Compare December 11, 2024 09:19
@bjhham
Copy link
Contributor Author

bjhham commented Dec 11, 2024

Benchmark: ktorio/ktor-benchmarks#49

@bjhham bjhham enabled auto-merge (squash) December 12, 2024 09:23
@bjhham bjhham force-pushed the bjhham/readLinePerfFix branch from 0a71fa9 to b1172f7 Compare December 16, 2024 08:22
@bjhham bjhham force-pushed the bjhham/readLinePerfFix branch from b1172f7 to 0590d42 Compare December 16, 2024 19:29
@bjhham bjhham merged commit f5363d5 into main Dec 16, 2024
15 of 16 checks passed
@bjhham bjhham deleted the bjhham/readLinePerfFix branch December 16, 2024 20:51
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.

2 participants