Skip to content

net/http: add request file upload benchmarks #30424

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

Closed
wants to merge 1 commit into from
Closed

net/http: add request file upload benchmarks #30424

wants to merge 1 commit into from

Conversation

vancluever
Copy link
Contributor

@vancluever vancluever commented Feb 27, 2019

This adds benchmarks to test file uploads using PUT requests.

It's designed to complement changes https://golang.org/cl/163599 and
https://golang.org/cl/163737, allowing an easy comparison of
performance before and after these changes are applied.

Updates #30377.

Co-authored-by: Emmanuel Odeke [email protected]

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Feb 27, 2019
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/163862 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1:

(3 comments)


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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/163862 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/163862 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@vancluever vancluever changed the title new/http: add request file upload benchmarks net/http: add request file upload benchmarks Feb 27, 2019
@gopherbot
Copy link
Contributor

Message from Chris Marchesi:

Patch Set 4:

(3 comments)

Ack! I imported the benchmarks from another repository and missed the update to remove the references for net/http. I've made the necessary changes as well and also fixed the change link in https://golang.org/cl/163737 as well. Cheers and thanks for catching all of that!


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

@gopherbot
Copy link
Contributor

Message from Emmanuel Odeke:

Patch Set 4:

Hello Chris, thanks for working on this. I have made an adaptation of your CL that'll allow you to test with various sizes too but also with some other updates, please see below:

const (
withTLS = true
noTLS = false
)

func BenchmarkFileAndServer_1KB(b *testing.B) {
benchmarkFileAndServer(b, 1<<10)
}

func BenchmarkFileAndServer_16MB(b *testing.B) {
benchmarkFileAndServer(b, 1<<24)
}

func BenchmarkFileAndServer_64MB(b *testing.B) {
benchmarkFileAndServer(b, 1<<26)
}

func benchmarkFileAndServer(b *testing.B, n int64) {
f, err := ioutil.TempFile(os.TempDir(), "go-bench-http-file-and-server")
if err != nil {
b.Fatalf("Failed to create temp file: %v", err)
}
defer func() {
f.Close()
os.RemoveAll(f.Name())
}()

if _, err := io.CopyN(f, rand.Reader, n); err != nil {
	b.Fatalf("Failed to copy %d bytes: %v", n, err)
}

b.Run("NoTLS", func(b *testing.B) {
	runFileAndServerBenchmarks(b, noTLS, f, n)
})
b.Run("TLS", func(b *testing.B) {
	runFileAndServerBenchmarks(b, withTLS, f, n)
})

}

func runFileAndServerBenchmarks(b *testing.B, tlsOption bool, f *os.File, n int64) {
handler := HandlerFunc(func(rw ResponseWriter, req *Request) {
defer req.Body.Close()

	nc, err := io.Copy(ioutil.Discard, req.Body)
	if err != nil {
		panic(err)
	}

	if nc != n {
		panic(fmt.Errorf("Copied %d Wanted %d bytes", nc, n))
	}
})

var cst *httptest.Server
if tlsOption == withTLS {
	cst = httptest.NewTLSServer(handler)
} else {
	cst = httptest.NewServer(handler)
}
defer cst.Close()

b.ResetTimer()
for i := 0; i < b.N; i++ {
	// Perform some setup.
	b.StopTimer()
	if _, err := f.Seek(0, 0); err != nil {
		b.Fatalf("Failed to seek back to file: %v", err)
	}
	b.StartTimer()

	req, err := NewRequest("PUT", cst.URL, ioutil.NopCloser(f))
	if err != nil {
		b.Fatal(err)
	}

	req.ContentLength = n
	// Prevent mime sniffing by setting the Content-Type.
	req.Header.Set("Content-Type", "application/octet-stream")

	res, err := cst.Client().Do(req)
	if err != nil {
		b.Fatalf("Failed to make request to backend: %v", err)
	}
	res.Body.Close()
	b.SetBytes(n)
}

}


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

This adds benchmarks to test file uploads using PUT requests.

It's designed to complement changes https://golang.org/cl/163599 and
https://golang.org/cl/163737, allowing an easy comparison of
performance before and after these changes are applied.

Updates #30377.

Co-authored-by: Emmanuel Odeke <[email protected]>
@gopherbot
Copy link
Contributor

Message from Chris Marchesi:

Patch Set 4:

Hello Chris, thanks for working on this. I have made an adaptation of your CL that'll allow you to test with various sizes too but also with some other updates, please see below:

Thanks for this Emmanuel! There are some things here for sure that I missed (ensuring not to count seeks) and things that I didn't know (that you can get a configured client from the test server now), so yeah, thank you!

I'll update the CL shortly and will add you as a co-author.


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

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. and removed cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. labels Mar 6, 2019
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/163862 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot pushed a commit that referenced this pull request Mar 6, 2019
This adds benchmarks to test file uploads using PUT requests.

It's designed to complement changes https://golang.org/cl/163599 and
https://golang.org/cl/163737, allowing an easy comparison of
performance before and after these changes are applied.

Updates #30377.

Co-authored-by: Emmanuel Odeke <[email protected]>

Change-Id: Ib8e692c61e1f7957d88c7101669d4f7fb8110c65
GitHub-Last-Rev: 242622b
GitHub-Pull-Request: #30424
Reviewed-on: https://go-review.googlesource.com/c/go/+/163862
Run-TryBot: Emmanuel Odeke <[email protected]>
Reviewed-by: Emmanuel Odeke <[email protected]>
@odeke-em
Copy link
Member

odeke-em commented Mar 9, 2019

This PR can now be closed at CL https://go-review.googlesource.com/c/go/+/163862 was merged.

@odeke-em odeke-em closed this Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants