net/http: use Copy in ServeContent if CopyN not needed

[costela]

This small PR allows optimizations made in io.Copy (like the use of
io.WriterTo) to be used in one possible path of http.ServeContent
(in case of a non-Range request).
This, in turn, allows us to skip the buffer allocation in io.Copy.

This is a retry of CL446276, reverted to address #61530 (whose root
cause has since been addressed)

[gopherbot]

This PR (HEAD: cc9afa5) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/555855.

Important tips:

• Don't comment on this PR. All discussion takes place in Gerrit.
• You need a Gmail or other Google account to log in to Gerrit.
• To change your code in response to feedback:
  • Push a new commit to the branch used by your GitHub PR.
  • A new "patch set" will then appear in Gerrit.
  • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
  • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
  • Multiple commits in the PR will be squashed by GerritBot.
• The title and description of the GitHub PR are used to construct the final commit message.
  • Edit these as needed via the GitHub web interface (not via Gerrit or git).
  • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
• See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

[gopherbot]

Message from Gopher Robot:

Patch Set 1:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Leo Antunes:

Patch Set 1:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!

[costela]

although the excelent benchmark by @AlexanderYastrebov seemed to indicate the underlying issue was solved, the higher-level benchmark by @mauri870 still shows a measurable slowdown (though not quite on the same level as originally observed; tested on current master):

goos: linux
goarch: amd64
pkg: go61530
cpu: 12th Gen Intel(R) Core(TM) i7-1250U
             │  old.bench  │             new.bench              │
             │   sec/op    │   sec/op     vs base               │
FileServe-12   17.27µ ± 6%   18.12µ ± 2%  +4.94% (p=0.007 n=10)

             │  old.bench   │               new.bench               │
             │     B/op     │     B/op      vs base                 │
FileServe-12   19.97Ki ± 0%   42.73Ki ± 0%  +114.01% (p=0.000 n=10)

             │ old.bench  │             new.bench              │
             │ allocs/op  │ allocs/op   vs base                │
FileServe-12   25.00 ± 0%   28.00 ± 0%  +12.00% (p=0.000 n=10)

Since the original syntetic benchmarks remain promising, this looks like another negative interaction with os.File.

I'll draft this PR while I investigate further. Sorry for the noise.

[costela]

TL;DR: benchmarks were skewed; this PR should cause no performance regressions 🎉 🤞
Nevermind, see below 😞

Longer:

The benchmark results above are skewed by the using httptest.ResponseRecorder, which causes a fallback to os.genericWriteTo for not being one of the supported targets for a WriteTo from an os.File.

Old (CopyN)

New (Copy)

The overhead is then caused by missing the optimization in copyBuffer for LimitReader. It pretty much disappears as soon as the the response size is bigger than the standard buffer size of 32k:

goos: linux
goarch: amd64
pkg: go61530
cpu: 12th Gen Intel(R) Core(TM) i7-1250U
             │  old.bench  │             new.bench              │
             │   sec/op    │   sec/op     vs base               │
FileServe-12   1.425m ± 2%   1.453m ± 5%  +1.95% (p=0.005 n=10)

             │  old.bench   │           new.bench            │
             │     B/op     │     B/op      vs base          │
FileServe-12   75.58Ki ± 3%   76.43Ki ± 3%  ~ (p=0.280 n=10)

             │ old.bench  │             new.bench              │
             │ allocs/op  │ allocs/op   vs base                │
FileServe-12   16.00 ± 0%   19.00 ± 0%  +18.75% (p=0.000 n=10)

Adding ReadFrom to the http.ResponseRecorder also negates the negative effects (I'd say this is an acceptable comparison with http.response, which also implements ReaderFrom):

adapted benchmark code

func BenchmarkFileServe(b *testing.B) {
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		http.ServeFile(w, r, "./go61530.test") // file size > 32k
	})

	r, _ := http.NewRequest("GET", "/file", http.NoBody)
	rw := recorderWithReaderFrom{httptest.NewRecorder()}

	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		handler.ServeHTTP(rw, r)
		rw.Body.Reset()
	}
}

type recorderWithReaderFrom struct {
	*httptest.ResponseRecorder
}

// ReadFrom implements io.ReaderFrom.
func (rr recorderWithReaderFrom) ReadFrom(r io.Reader) (n int64, err error) {
	return rr.Body.ReadFrom(r)
}

goos: linux
goarch: amd64
pkg: go61530
cpu: 12th Gen Intel(R) Core(TM) i7-1250U
             │  old.bench  │           new.bench           │
             │   sec/op    │   sec/op     vs base          │
FileServe-12   1.018m ± 7%   1.032m ± 3%  ~ (p=0.165 n=10)

             │  old.bench   │           new.bench            │
             │     B/op     │     B/op      vs base          │
FileServe-12   32.45Ki ± 3%   31.10Ki ± 7%  ~ (p=0.165 n=10)

             │ old.bench  │             new.bench              │
             │ allocs/op  │ allocs/op   vs base                │
FileServe-12   15.00 ± 0%   18.00 ± 0%  +20.00% (p=0.000 n=10)

This means this PR shouldn't harm performance for the io.File case, and should enable WriteTo from sources that support if (e.g. x/exp/mmap, hopefully 🤞)

[costela]

AFAICT the PR should actually improve performance for the realistic io.File case because ReadFrom in http.response will have access to the io.File (instead of io.LimitReader)? 🤔

[costela]

This still causes a regression in the io.File case:

1. io.Copy tries to use WriteTo from the io.File (not available if using io.CopyN), which fails because http.response doesn't implement syscall.Conn, so we use genericWriteTo
2. that calls a second round of io.Copy, now with a wrapped io.File, so we skip WriteTo and go straight to http.response.ReadFrom.
3. that then cannot use sendFile, because we got a fileWithoutWriteTo as source, so we're back to genericReadFrom
4. finally, a third round of io.Copy, which just falls back to buffered copy.

[gopherbot]

This PR (HEAD: 88ec1fb) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/go/+/555855.

Important tips:

• Don't comment on this PR. All discussion takes place in Gerrit.
• You need a Gmail or other Google account to log in to Gerrit.
• To change your code in response to feedback:
  • Push a new commit to the branch used by your GitHub PR.
  • A new "patch set" will then appear in Gerrit.
  • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
  • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
  • Multiple commits in the PR will be squashed by GerritBot.
• The title and description of the GitHub PR are used to construct the final commit message.
  • Edit these as needed via the GitHub web interface (not via Gerrit or git).
  • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
• See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

[costela]

With the latest commit the regression is solved. We now see a working ReadFrom+sendFile in the io.File case:

However, since there are currently multiple proposals open around the intersection of WriterTo and io.Copy (e.g. #16474, #58331, #56742), it may be necessary to split the last commit into a proposal.

Alternatively, we could also add an internal interface that is only implemented by io.File, so that all type checks can be done against it. This would ensure this change has no side-effects outside of the stdlib. In its current form, the change has an unlikely impact, but could still theoretically accidentally match some interface in the wild.

[gopherbot]

Message from Damien Neil:

Patch Set 7: Commit-Queue+1

(2 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Go LUCI:

Patch Set 7:

Dry run: CV is trying the patch.

Bot data: {"action":"start","triggered_at":"2024-02-27T23:11:06Z","revision":"0ae37daaddf1821075c94dcfb5658d9e54b88479"}

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Damien Neil:

Patch Set 7: -Commit-Queue

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Go LUCI:

Patch Set 7:

This CL has passed the run

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Go LUCI:

Patch Set 7: LUCI-TryBot-Result+1

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/555855.
After addressing review feedback, remember to publish your drafts!