-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
net/http: improve performance for parsePostForm #14655
Comments
Actually, instead of a (In the future, it might be nice to not even allocate for the common map keys, since most http.Handlers receiving POSTs will be getting the same map keys repeatedly. But there's no great place to stash that cache, since we don't know which handler the request is for (yet). We might know that later, if #14660 happens, since we could associate a Handler with a Request via context keys... but that's just thinking ahead and not needed for addressing this garbage) |
You mean for instance adding a |
Without adding any new public API, though. |
OK so to avoid adding a public API I've added a
Is that what you had in mind ? Because running the same benchmark gives me worse results than the last 2 benchmarks :
Just by looking at this result we can see that it's 3 times slower and allocates as much as the original implementation (since this time it only does 500 repetitions vs 2000 last time)
Is there any particular reason you don't want to go the If you still do, is there anything I've misinterpreted from your guidelines that would worsen the benchmark ? |
If you want me to do a code review, please send a change in Gerrit. See https://golang.org/doc/contribute.html Otherwise I can look into this later, writing a fast version. There are a number of things not fast about your code. |
Just sent the change in Gerrit. Looking forward for your code review :) |
CL https://golang.org/cl/20301 mentions this issue. |
@odeke-em FYI I've just pushed my changes with the benchmark included in the commit message |
CL https://golang.org/cl/30454 mentions this issue. |
CL https://golang.org/cl/30470 mentions this issue. |
This reverts commit 59320c3. Reasons: This CL was causing failures on a large regression test that we run within Google. The issues arises from two bugs in the CL: * The CL dropped support for ';' as a delimiter (see https://golang.org/issue/2210) * The handling of an empty string caused an empty record to be added when no record was added (see https://golang.org/cl/30454 for my attempted fix) The logic being added is essentially a variation of url.ParseQuery, but altered to accept an io.Reader instead of a string. Since it is duplicated (but modified) logic, there needs to be good tests to ensure that it's implementation doesn't drift in functionality from url.ParseQuery. Fixing the above issues and adding the associated regression tests leads to >100 lines of codes. For a 4% reduction in CPU time, I think this complexity and duplicated logic is not worth the effort. As such, I am abandoning my efforts to fix the existing issues and believe that reverting CL/20301 is the better course of action. Updates #14655 Change-Id: Ibb5be0a5b48a16c46337e213b79467fcafee69df Reviewed-on: https://go-review.googlesource.com/30470 Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
My main concern is using
ioutil.ReadAll
everytime theio.Reader
is read inparsePostForm
since it will then triggerbytes.makeSlice
and allocate new space each and every time the function is called. Very similar to my open PR to @bradfitz's memcache library (bradfitz/gomemcache#45)Instead I think using a
sync.Pool
of*bytes.Buffer
would be a fairly better solution.I've implemented the solution and added a benchmark to show you the performance improvements.
First of, here's the benchmark:
Now here's the first run of the benchmark before the improvement:
You can see that
ioutil.ReadAll(reader)
at line 898 is eating17.31GB
.Now here's the result of the benchmark after the improvement I suggest:
The very same command, this time at line 911, now eats only
359.40MB
for the same number of requests.If you need any extra information please let me know.
If you think this performance proposal is sound I'll mail the change for review using
codereview
.Cheers
Quentin
The text was updated successfully, but these errors were encountered: