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
proposal: x/net/http2: add Framer.WriteDataN method #66655
Comments
CC @neild |
I think this would need slight tweaks. Namely, from the docs of
In this case, the new |
I think my first question here is whether the better approach is for gRPC to copy As you point out, the |
Either of these would be nice, but not mandatory IMO. We can own the logic to pass the appropriate slices and maintain the size constraints ourselves. (I'm not even sure if the framer is currently aware of the max frame size, so this might be required anyway.)
That's not an unreasonable option. On the other hand, if there are bugs or vulnerabilities discovered, it's nice when code is centralized to also centralize the fixes to those problems. Similarly, if a more performant methodology could be found and agreed upon, then those benefits can be shared by everyone using a standard(ish) library. |
By that argument, gRPC should be using the entire HTTP/2 implementation from x/net/http2, not just the framer. Perhaps that would have been a good idea, but that ship sailed long ago. The proposal here ( |
Brad tried that before I even joined the team. It's still in the code, and some users use it. I would love to live in that world, but alas, it's not this one. I'm not sure whether the net/http server supports all the things gRPC needs to support, like BDP estimation, configurable keepalive pings, max connection age, etc, but if it doesn't, I am guessing it would be even more painful to add those features than adding a simple, optional function to the framer.
We don't have the resources to embark on a full framer rewrite at this time. So our choices are to fork it and add this simple method ourselves, or to add it to x/net/http2. I'd really rather not fork the whole package for something so small. If you're saying that's the only path forward, we will consider it, but it feels wrong. This is a very trivial extension of the existing API and would not impose any new limitations of future work. |
Is there a path forward here? If I send a CL, would it be considered? |
An alternate possibility is that you add this one function to your own implementation:
~20 lines, fairly straightfoward. (I did omit checking that the data size is < 2^24, so a few more lines for that.) This requires that you have access to the framer's This will also be rather more efficient than adding a I might modify the function signature a bit, though, to avoid the need to build a
Another question is whether it's really all that useful to combine these writes into one frame; the per-frame overhead is 8 bytes, which isn't nothing, but splitting one chunk of data across two frames doesn't seem all that expensive. I'll assume you've measured this, but if you haven't it might be worth thinking about. |
This proposal has been added to the active column of the proposals project |
Proposal Details
To write a data frame, the
x/net/http2.Framer
requires the data contents to first be accumulated into a single contiguous buffer. However, this is not always possible without requiring a copy. See this real-life example where a copy is required in order to construct the frame's contents before writing it. If we had an API like the following, this copy could be avoided:The implementation could be as simple as changing the append here into an append that loops over the slice of buffers instead of just the one buffer.
The gRPC code could then be changed to remove the copy and pass the 5-byte header buffer and the data buffer directly to the new method.
I'd also love to discuss other changes that could make the HTTP/2 Framer even more zero-copy friendly by transferring buffer ownership instead of using
io.Reader
/io.Writer
, but the proposal above would be a simple step that provides real incremental performance.cc @PapaCharlie, @atollena
The text was updated successfully, but these errors were encountered: