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: mime/multipart.Reader should use sync.Pool and reuse bytes.Buffers to reduce memory allocation pressure #21516
Comments
Thank you @SunRunAway for reporting the issue. Would you mind elaborating more on what you would like to have this issue to solve? |
hi, @odeke-em |
Change https://golang.org/cl/56990 mentions this issue: |
One small issue is that we try to avoid using a sync.Pool for buffers that are ever observed by user space. So if Reader calls some arbitrary io.Reader's Read method to fill in the buffer, it should not go back into the pool, to avoid a dangling pointer bug in that Reader from corrupting some future unrelated call. |
@rsc Sorry, I cannot understand. Could you give an example please? |
Alternative fixes would involve making the multipart.Reader re-usable by supporting a Reset method and store a bytes.Buffer as well as a buffer for io.CopyBuffer inside it for re-use. So the http library can do the re-use and pooling on request level. |
Hi, do you have any suggestion to do or decision about this issue & CL? |
This restriction sounds strange. It is based on a weird corner case that should never happen in real code. How many update: func Read(p []byte) (int, error) {
go fillBuffer(p)
...
} But it is already broken:
|
In general all reasons not to perform optimizations fall into that category. I'd still like to keep bad code in one package from causing mysterious failures in an unrelated package, and that's the motivation for the restriction I described. On the other hand it sounds like you have a significant opportunity to make your server faster. The question is whether there's a way to do it at a slightly higher level and avoid the possible corruption in buggy code. |
The |
Per #21516 (comment) above, do you have a revised API suggestion? |
Timed out in state WaitingForInfo. Closing. (I am just a bot, though. Please speak up if this is a mistake or you have the requested information.) |
go/src/mime/multipart/formdata.go
Line 59 in 59413d3
We have a upload server using req.ParseMultipartForm. GOGCTRACE tells us gabarage collector takes 8% cpu usage and the heap profile tells us mine/multipart.Reader.ReadForm allocates lots of memory.
The text was updated successfully, but these errors were encountered: