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 copying continuation byte arrays by using ByteString internally #1923

Closed
alecgrieser opened this issue Nov 18, 2022 · 0 comments · Fixed by #1924
Closed

Avoid copying continuation byte arrays by using ByteString internally #1923

alecgrieser opened this issue Nov 18, 2022 · 0 comments · Fixed by #1924
Assignees
Labels
performance Performance issues

Comments

@alecgrieser
Copy link
Contributor

The RecordCursorContinuation class has a toBytes() method which returns a byte[] and a toByteString() method which returns a ByteString. Internally, there are quite a few cases where we construct the byte representation by constructing a protobuf, often with child continuations that serialize their continuation to a byte[], which we then wrap in a ByteString and hand to the protobuf serializer.

We should be able to skip that intermediate state by using toByteString on the continuation object. Some care does need to be taken, though, as some implementations of toByteString() are (more-or-less) ByteString.copy(toBytes()), which could induce additional copying. But assuming we can tidy that up, this should remove some byte copies and object creation.

@alecgrieser alecgrieser added the performance Performance issues label Nov 18, 2022
@alecgrieser alecgrieser self-assigned this Nov 18, 2022
alecgrieser added a commit to alecgrieser/fdb-record-layer that referenced this issue Nov 18, 2022
… 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 added a commit to alecgrieser/fdb-record-layer that referenced this issue Nov 21, 2022
… 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.
MMcM added a commit that referenced this issue Nov 21, 2022
Resolves #1923: Avoid copying continuation byte arrays by using ByteString internally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance issues
Projects
None yet
1 participant