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.

Sign up for GitHub

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

Jump to bottom

UnsafeBufferOperations.forEachSegment implementation #383

Merged
merged 5 commits into from
Sep 9, 2024
Merged

UnsafeBufferOperations.forEachSegment implementation #383

merged 5 commits into from
Sep 9, 2024

Conversation

qwwdfsad
Copy link
Member

@qwwdfsad qwwdfsad commented Sep 4, 2024

More concise replacement for 'iterate' function

@qwwdfsad
UnsafeBufferOperations.forEachSegment implementation
03f7393 
More concise replacement for 'iterate' function
@qwwdfsad
Copy link
Member Author

qwwdfsad commented Sep 4, 2024 •
edited
Loading

This is a draft, please take a look.

If you are good with the idea, I'll deprecate iterate functions, write tests for forEach and re-tweak samples.
The further deprecation will also allow us to completely remove BufferIterationContext, shrinking the overall API surface a bit

qwwdfsad
qwwdfsad commented Sep 4, 2024
View reviewed changes
core/jvm/src/BuffersJvm.kt Outdated
}
curr = ctx.next(curr) ?: break
currentOffset = 0
var firstSegmentHandled = false
Copy link
Member Author

@qwwdfsad qwwdfsad Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This one is unfortunate, please take a closer look.
It also comes with a few more ifs this time, not sure it's a measurable effect though

@fzhinkin
Copy link
Collaborator

fzhinkin commented Sep 4, 2024

@qwwdfsad Nicely done!
I love the idea of having a more concise API solving a problem that should be the most common - scanning the whole buffer. However, there are certain cases when a more granular control over segments advancement could be beneficial.

Buffer::copyTo did not became prettier after switching to forEachSegment, but it looks OK.
But in algorithms like Okio's select lack of a granular control could hurt much more. There, the algorithm scans over continues stripes of bytes that could cross segment boundaries. Currently, Okio detects segment-has-ended condition, fetches a new portion of data in place and continue iteration. The same could be achieved with our Unsafe API and iterate + next. With forEachSegment it is still possible to implement the same algorithm, but implementation's cognitive complexity could grow.

I'm all in with adding a new API, but I have some doubts about existing API's deprecation.

@qwwdfsad
Revert copyTo implementation as we won't be able to avoid 'iterate' f…
a4ba11a 
…unction

* Add a single sample for 'iterate' and 'BufferIterationContext'
* Rewrite previous samples using 'forEachSegment'
* Remove 'forEachSegment' overload with offset, 'iterate' is recommended for that
@qwwdfsad
Tests for 'forEachSegment'
dd06eab
@qwwdfsad qwwdfsad marked this pull request as ready for review September 5, 2024 17:33
@qwwdfsad @fzhinkin
Update core/common/test/samples/unsafe/unsafeSamples.kt
8a7b407 
Co-authored-by: Filipp Zhinkin <filipp.zhinkin@jetbrains.com>
fzhinkin
fzhinkin approved these changes Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@fzhinkin fzhinkin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@qwwdfsad qwwdfsad merged commit eb7a5d2 into develop Sep 9, 2024
1 check passed
@qwwdfsad qwwdfsad deleted the unsafe-ops-for-each branch September 9, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzhinkin fzhinkin fzhinkin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qwwdfsad @fzhinkin