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

Make Content.Chunk a RetainableByteBuffer #11798

Closed
wants to merge 80 commits into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented May 16, 2024

Opening this PR against 12.1.x rather than 12.0.x, replacing #11598

I'm just going to summarize the objectives of this PR:

  • Homogenize the APIs used for buffer manipulation in utils, aggregators, accumulators and buffers. Currently we have several different naming conventions and semantics (e.g. is the buffer consumed or not).
  • Reduce the API impedance between key abstractions. Buffers from Content.Sources should be compatible with buffers from ByteBufferPools and both should be easily written to Content.Sinks
  • Separate the read-only APIs from read-write APIs. The API used should indicate if buffer mutation is permitted or not.
  • protect mutable APIs from being applied to retained buffers
  • Avoid or hide modal APIs. Abstract away from the BB fill/flush mode issues so that these are rarely a concern for most buffer manipulations.
  • Where possible, internal implementations should leave BBs in the appropriate mode to avoid excess flipping
  • Remove unnecessary copies and encourage retention where possible and desirable.
  • Create common mechanisms that could allow heuristics decisions regarding copy vs retain, so that common usage does not need to make a hard coded choice.
  • Compatible with current buffer and API usage. We want evolution not revolution.
  • Avoid the complaints about BufferUtil and flipToFill and flipToFlush

@gregw
Copy link
Contributor Author

gregw commented May 16, 2024

@lorban said:

What I generally like:

  • The writeTo() pattern as the less we have to call getByteBuffer() the better.
  • The append() pattern for the same reason as above.
  • The split between RBB and RBB.Mutable to make it clearer when some API is meant to consume or produce data.> + The strong abstraction of the Appender / Aggregator.

What makes me cautions:

  • The multiplication of append() overloads that takes byte[] with and without offset, ByteBuffer, RBB and so on, as it makes the API less concise. Potential ditto for writeTo().
  • The fact that RBB and Content.Sink aren't in the same module so we cannot design an API where RBB exposes API using Content.Sink.
  • Chunk becoming a RBB subclass looks both elegant and unnatural. I have to ponder that a bit more as I have no objection besides a bad gut feeling.

If we're considering those changes for 12.1, I'm thinking we should charge ahead as it is the perfect time to make changes to semi-internal APIs.

In response:

  • I've moved this to 12.1, but I think the branch needs rebasing....
  • I have kept the byte[] APIs to a minimum. Only really for the put replacements, which have no reason to wrap as BB or RBB
  • RBB and Content.Sink are both in org.eclipse.jetty.io ??? RBB only uses Content.Sink in writeTo and things like EndPoint now implement Sink or are easy to wrap as a sink.
  • Chunk has buffered content and is Retainable. So there is very much a natural fit. What parts feel unnatural to you? i.e. what RBB methods do you think do not apply to a Chunk?

@gregw gregw changed the base branch from jetty-12.1.x to jetty-12.0.x May 16, 2024 05:28
@gregw
Copy link
Contributor Author

gregw commented May 16, 2024

I've switched this back to 12.0.x for now, until 12.1.x is stable and I've done the rebase work

@gregw
Copy link
Contributor Author

gregw commented May 16, 2024

closing to go again after rebase in new branch

@gregw gregw closed this May 16, 2024
@gregw
Copy link
Contributor Author

gregw commented May 16, 2024

replaced by #11801

@joakime joakime deleted the fix/jetty-12/10541/byteBufferAccumulator0 branch June 4, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant