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

Comments

Projects
None yet
4 participants
@go101
Copy link

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

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

@agnivade

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

@agnivade

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.