Skip to content
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: Request.WithContext is slow #28737

Closed
go101 opened this issue Nov 12, 2018 · 6 comments
Closed

net/http: Request.WithContext is slow #28737

go101 opened this issue Nov 12, 2018 · 6 comments

Comments

@go101
Copy link

go101 commented Nov 12, 2018

What version of Go are you using (go version)?

go version go1.11.2 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

I write a http router package: https://github.com/go101/tinyrouter
The benchmark result shows it spends about 3x times (24136 ns / 7285 ns/op) to match a pattern as HttpRouter.
After some profiling, I found the slowness is mainly caused by one line in my package:

req = req.WithContext(context.WithValue(req.Context(), paramsKeyType{}, Params{path, tokens}))

This line spends about 11000 ns. In the 11000 ns, 8000ns is spent on the req.WithContext() call.

If my package doesn't return URL path params through context (as HttpRouter does), then it only 30% slower than HttpRouter.

Considering that sometimes there may be more values to be appended into the context, it will spend more time. So I think we really need an alternative faster way to append key-values to http request context.

@go101
Copy link
Author

go101 commented Nov 12, 2018

I counted the number of native word copied in WithContext function. The number is 43+.

@bradfitz bradfitz changed the title net/http: http.Request.WithContext is very slow. Need an alternative way to set context key-value net/http: Request.WithContext is slow Nov 12, 2018
@bradfitz bradfitz added this to the Unplanned milestone Nov 12, 2018
@OneOfOne
Copy link
Contributor

.WithContext returns a full copy of the request + URL, so it's expected to be heavy.

@agnivade
Copy link
Contributor

agnivade commented Mar 8, 2019

I don't see much that can be done here. We are just copying the request, context and the url of the original request.

Any optimization will deviate from the original behavior, for eg. like just updating the context to the same request. And it seems that is exactly what you are doing too. You are re-assigning the new request to the old request.

@go101
Copy link
Author

go101 commented Mar 8, 2019

@agnivade
Copy link
Contributor

agnivade commented Mar 8, 2019

That's what I said. You cannot optimize this further without breaking original behavior. For v2, we can break behavior and possibly optimize this. Perhaps you want to add a note for optimizing context behavior in #5465 ?

@bradfitz
Copy link
Contributor

bradfitz commented Mar 8, 2019

I'm going to close this as unfortunate for now. We can address this in a future rewrite of the client package.

For now, if you need to make Requests with Contexts cheaply, you can have a global variable holding an http.Request zero value and then use its WithContext to make your new requests. For instance, if you wanted to make a version of net/http.NewRequest that took a context, users today can write:

var zeroRequest http.Request

func NewRequestContext(ctx context.Context, method, url string, body io.Reader) (*Request, error) {
	u, err := parseURL(url) // Just url.Parse (url is shadowed for godoc).
	if err != nil {
		return nil, err
	}
...
	req := zeroRequest.WithContext(ctx)
        req.URL = u
        req.Method = method
        ...
}

And that's pretty efficient. (no deep cloning of an existing URL)

@bradfitz bradfitz closed this as completed Mar 8, 2019
@golang golang locked and limited conversation to collaborators Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants