Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,19 @@ public ReleasableBytesReference retain() {
return this;
}

/**
* Same as {@link #slice} except that the slice is not guaranteed to share the same underlying reference count as this instance.
* This method is equivalent to calling {@code .slice(from, length).retain()} but might be more efficient through the avoidance of
* retaining unnecessary buffers.
*/
public ReleasableBytesReference retainedSlice(int from, int length) {
if (from == 0 && length() == length) {
return retain();
}
final BytesReference slice = delegate.slice(from, length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny enough I found this looking into a different optimization that wouldn't build composite bytes references out of releasable ones by unwrapping. While that has its runtime advantages, this change seems much more helpful by saving memory and optimisations like #124947 (if taken a little further that is) could leverage this for even bigger gains.
As it stands, this change actually is quite helpful already when working with SearchHit instances sliced out of large buffers at the very least (there might be other cases where it's helpful as well). Also, it avoids a pointless round of ref-counting in the decoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could also be taken further by making the logic aware of situations where the slice itself is made up of a subset of the releasable references that the parent instance consists of and only retaining those. Obviously then considerably increasing the complexity of the implementation for a likely limited gain in corner cases.

if (slice instanceof ReleasableBytesReference releasable) {
return releasable.retain();
}
refCounted.incRef();
return new ReleasableBytesReference(slice, refCounted);
}
Expand Down Expand Up @@ -131,10 +139,17 @@ public int length() {
return delegate.length();
}

/**
* {@inheritDoc}
*
* The returned bytes reference will share the reference count of this instance and as such any ref-counting operations on the return
* are shared with this instance and vice versa. Using {@link #retainedSlice(int, int)} might be more efficient in situations where the
* return of this method is subsequently retained by increasing its ref-count.
*/
@Override
public BytesReference slice(int from, int length) {
public ReleasableBytesReference slice(int from, int length) {
assert hasReferences();
return delegate.slice(from, length);
return new ReleasableBytesReference(delegate.slice(from, length), refCounted);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,7 @@ public int internalDecode(ReleasableBytesReference reference, CheckedConsumer<Ob
bytesConsumedThisDecode += maxBytesToConsume;
bytesConsumed += maxBytesToConsume;
if (maxBytesToConsume == remainingToConsume) {
try (ReleasableBytesReference retained = reference.retainedSlice(0, maxBytesToConsume)) {
fragmentConsumer.accept(retained);
}
fragmentConsumer.accept(reference.slice(0, maxBytesToConsume));
} else {
fragmentConsumer.accept(reference);
}
Expand Down