Parse the url only once per Client

[mattrobenolt]

This stood out on flame graphs due ultimately to the http.NewRequestFromContext call, which ends up calling url.Parse and this happens on every single RPC call.

I did an optimization in #447 which similarly, and this is a natural extension of this.

We can just parse the url string once during NewClient, validate at that point, then tag along the parsed *url.URL everywhere else and never use it as a string again. This value never mutates through the lifetime of a Client and isn't publicly available on any structs.

Before:

After:

[mattrobenolt]

This is mirroring what's expected in stdlib. I hate it, but it is what it is. Happy to comment this up, but we can really skip any of the helpers and conveniences that NewRequestWithContext provides since our inputs are well known.

[akshayjshah]

Makes sense. This is fine with me, but I would like a few comments and a link the the implementation of NewRequestWithContext. (The hard-coded HTTP/1.1 constants are offputting to read!)

[mattrobenolt]

This is erroring on the side of caution since we can't really control what an http.RoundTripper may do, and I'm not sure if it's expected that a RoundTripper is not allowed to mutate this, or is expected to make it's own copy if it were?

Doing this at least mirrors the safety we had when we passed along a string and it was parsed every time.

[akshayjshah]

RoundTripper implementations aren't supposed to mutate the URL:

// RoundTrip should not modify the request, except for
// consuming and closing the Request's Body. RoundTrip may
// read fields of the request in a separate goroutine. Callers
// should not mutate or reuse the request until the Response's
// Body has been closed.

In practice, I don't think many people read the docs very carefully so this extra safety is a good idea.

[mattrobenolt]

Just to follow up on this, we've been running this patch in production now and the improvement is measurable in that the url.Parse time is entirely gone.

This isn't significant on it's own, but since this is per-RPC, it's definitely a relatively expensive operation when the URL doesn't change. It also skips the method validation http.validateMethod since we're just using the constants and know it's valid.

Creating the io.Pipe() now what dominates here, and that's pretty essential.

[akshayjshah]

Creating the io.Pipe() now what dominates here, and that's pretty essential.

Weeeeeelllllll.....it's not really. IIRC most of your RPCs are unary, so we don't actually need the pipe or the extra goroutine - we've got all the data in memory and can easily pass it as the request body. We only need all the code in duplex_http_call.go for streaming requests; unary and server streaming RPCs could use a different, faster implementation of StreamingClientConn. We had code written for this early in Connect's life, and IIRC it was ~30% faster.

I ended up deciding to drop the alternate code path and exercise the request-streaming code on every RPC because it's by far the most complex code in connect-go, and I wanted to flush out any bugs quickly. I think we're past that point now, so we can at least consider a PR to speed up unary and server streaming calls.

[mattrobenolt]

Oh, if we can drop this and the goroutine, that'd be a decently large win. I guess it makes sense this is only for a bidirectional stream. I'd suspect it can be dropped also for a server steam only with a unary request? I'd be down to look into this unless you have a good grasp or what would be involved.

Regardless, deduping the url.Parse is still good.

[akshayjshah]

Oh, if we can drop this and the goroutine, that'd be a decently large win. I guess it makes sense this is only for a bidirectional stream. I'd suspect it can be dropped also for a server steam only with a unary request? I'd be down to look into this unless you have a good grasp or what would be involved.

Yep, it should also work for server streaming. IIRC you'd need a different implementation of StreamingClientConn that errors when Send is called more than once; other than that it shouldn't be too too bad. We'd love it if you took a stab at a PR!

Agreed that this PR is good either way :)

[akshayjshah]

Definitely much nicer! Appreciate the optimization, especially since it also simplifies some irritating error handling.

Please do fix the lint errors about variable name length - our house style frowns upon 1-2 letter variable names unless their scope is very small. Once the lint errors are fixed and the NewRequestWithContext comments are added in, this is good to merge.

[mattrobenolt]

@akshayjshah pushed both changes.

In my defense, I copy/pasted cloneURL from stdlib. I'm sure you preferred that over me doing a //go:linkname. :)