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
Resolves #1923: Avoid copying continuation byte arrays by using ByteString internally #1924
Merged
MMcM
merged 1 commit into
FoundationDB:main
from
alecgrieser:continuation-to-byte-string
Nov 21, 2022
Merged
Resolves #1923: Avoid copying continuation byte arrays by using ByteString internally #1924
MMcM
merged 1 commit into
FoundationDB:main
from
alecgrieser:continuation-to-byte-string
Nov 21, 2022
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
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
MMcM
approved these changes
Nov 18, 2022
MMcM
requested changes
Nov 18, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, but some test failures look to be actual regressions.
… using ByteString internally This updates the logic in the `RecordCursorContinuation` objects to avoid serializing a continuation all the way to a `byte[]` if it is not necessary. For example, when constructing protobuf objects that back continuations, child continuations can be constructed by calling `.toByteString()` on the continuation and passing that to the protobuf builder instead of serializing all the to a `byte[]` and then wrapping it back in a `ByteString`. This also makes sure that (as much as possible) the cursor implementations are more efficient when constructing a `ByteString`. For example, for the proto-based continuations, it should be able to return `.toByteString()` on the protobuf object instead of serializing the _protobuf_ all the way to a `byte[]` and then wrapping it back up, which some implementations did previously. Adopters who need a `ByteString` rather than a `byte[]` (for example, to pack the continuation into a protobuf) are also encouraged to switch to the newer method. This resolves FoundationDB#1923.
alecgrieser
force-pushed
the
continuation-to-byte-string
branch
from
November 21, 2022 13:00
f3ea6ba
to
d7d0fe3
Compare
Please retry analysis of this Pull-Request directly on SonarCloud. |
Kudos, SonarCloud Quality Gate passed! |
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
MMcM
approved these changes
Nov 21, 2022
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 updates the logic in the
RecordCursorContinuation
objects to avoid serializing a continuation all the way to abyte[]
if it is not necessary. For example, when constructing protobuf objects that back continuations, child continuations can be constructed by calling.toByteString()
on the continuation and passing that to the protobuf builder instead of serializing all the to abyte[]
and then wrapping it back in aByteString
. This also makes sure that (as much as possible) the cursor implementations are more efficient when constructing aByteString
. For example, for the proto-based continuations, it should be able to return.toByteString()
on the protobuf object instead of serializing the protobuf all the way to abyte[]
and then wrapping it back up, which some implementations did previously.Adopters who need a
ByteString
rather than abyte[]
(for example, to pack the continuation into a protobuf) are also encouraged to switch to the newer method.This resolves #1923. This also follows up from #1921, which introduced the zero copy
ByteString
to avoid copying immutable arrays.