Add byte array pooling to nio http transport #31349
Conversation
Tim-Brooks
added
>non-issue
review
:Distributed Coordination/Network
|
Pinging @elastic/es-core-infra |
| } | ||
|
|
||
| private Page duplicate() { | ||
| referenceCount.incrementAndGet(); |
There was a problem hiding this comment.
I am a little concerned about this. What if the referenceCount is already 0? I think this should implement AbstractRefCounted? I guess that is not available in this lib? I would love to move that class to core and replace the AlreadyClosedException with IllegalStateException and use it here?!
There was a problem hiding this comment.
I do wonder why we need to refCount this. I think it would be good to have some explanation why this is is needed. I also assume if we miss returning a page we will fail tests because not all byte blocks are released right?
There was a problem hiding this comment.
I do wonder why we need to refCount this.
It's to integrate with netty. Netty can hold onto bytes internally and we don't know when it is down with them without overriding its buffer.
I also assume if we miss returning a page we will fail tests because not all byte blocks are released right?
Yeah normal mock page recycler stuff
I think this should implement AbstractRefCounted?
I will do this but it will cause the introduction of more layers of objects. Can Page be reference counted? Yes. But when a return a Page, it needs to be a different Page object because the ByteBuffer needs to be different. So internally we will need to create like a Releaser object that is reference counted that is passed around to the different pages.
|
|
||
| private static ByteBuf byteBufFromPage(InboundChannelBuffer.Page page) { | ||
| ByteBuffer buffer = page.getByteBuffer(); | ||
| assert !buffer.isDirect() && buffer.hasArray() : "Must be a heap buffer with an array"; |
| assertEquals(0, integer.get()); | ||
| secondBuf.release(); | ||
| assertEquals(pageCount, integer.get()); | ||
| } |
There was a problem hiding this comment.
maybe add some basic tests that this buffer works :)
|
@s1monw Changes made. |
|
run gradle build tests |
1 similar comment
|
run gradle build tests |
* master: Add get stored script and delete stored script to high level REST API - post backport fix Add get stored script and delete stored script to high level REST API (#31355) Core: Combine Action and GenericAction (#31405) Fix reference to XContentBuilder.string() (#31337) Avoid sending duplicate remote failed shard requests (#31313) Fix defaults in GeoShapeFieldMapper output (#31302) RestAPI: Reject forcemerge requests with a body (#30792) Packaging: Remove windows bin files from the tar distribution (#30596) Docs: Use the default distribution to test docs (#31251) [DOCS] Adds testing for security APIs (#31345) Clarify that IP range data can be specified in CIDR notation. (#31374) Use system context for cluster state update tasks (#31241) Percentile/Ranks should return null instead of NaN when empty (#30460) REST high-level client: add validate query API (#31077) Move language analyzers from server to analysis-common module. (#31300) [Test] Fix :example-plugins:rest-handler on Windows Expose lucene's RemoveDuplicatesTokenFilter (#31275) Reload secure settings for plugins (#31383) Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381) Ensure we don't use a remote profile if cluster name matches (#31331) [TEST] Double write alias fault (#30942) [DOCS] Fix version in SQL JDBC Maven template [DOCS] Improve install and setup section for SQL JDBC SQL: Fix rest endpoint names in node stats (#31371) Support for remote path in reindex api - post backport fix Closes #22913 [ML] Put ML filter API response should contain the filter (#31362) Support for remote path in reindex api (#31290) Add byte array pooling to nio http transport (#31349) Remove trial status info from start trial doc (#31365) [DOCS] Adds links to release notes and highlights add is-write-index flag to aliases (#30942) Add rollover-creation-date setting to rolled over index (#31144) [ML] Hold ML filter items in sorted set (#31338) [Tests] Fix edge case in ScriptedMetricAggregatorTests (#31357)
This is related to #28898. This PR implements pooling of bytes arrays
when reading from the wire in the http server transport. In order to do
this, we must integrate with netty reference counting. That manner in
which this PR implements this is making Pages in InboundChannelBuffer
reference counted. When we accessing the underlying page to pass to
netty, we retain the page. When netty releases its bytebuf, it releases
the underlying pages we have passed to it.