Parse the url only once per Client (#467)

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:
<img width="1026" alt="image"
src="https://user-images.githubusercontent.com/375744/219882639-b05416ee-3a8f-4b38-9716-9d66a34eeffc.png">

After:
<img width="1177" alt="image"
src="https://user-images.githubusercontent.com/375744/220477453-c1b48350-665b-495b-a070-3efb08e53067.png">

client.go

@@ -17,8 +17,11 @@ package connect
 import (
  "context"
  "errors"
+ "fmt"
  "io"
  "net/http"
+ "net/url"
+ "strings"
 )
 
 // Client is a reusable, concurrency-safe client for a single procedure.
@@ -55,7 +58,7 @@ func NewClient[Req, Res any](httpClient HTTPClient, url string, options ...Clien
  Protobuf: config.protobuf(),
  CompressMinBytes: config.CompressMinBytes,
  HTTPClient: httpClient,
- URL: url,
+ URL: config.URL,
  BufferPool: config.BufferPool,
  ReadMaxBytes: config.ReadMaxBytes,
  SendMaxBytes: config.SendMaxBytes,
@@ -171,6 +174,7 @@ func (c *Client[Req, Res]) newConn(ctx context.Context, streamType StreamType) S
 }
 
 type clientConfig struct {
+ URL *url.URL
  Protocol protocol
  Procedure string
  CompressMinBytes int
@@ -184,9 +188,14 @@ type clientConfig struct {
  SendMaxBytes int
 }
 
-func newClientConfig(url string, options []ClientOption) (*clientConfig, *Error) {
- protoPath := extractProtoPath(url)
+func newClientConfig(rawURL string, options []ClientOption) (*clientConfig, *Error) {
+ url, err := parseRequestURL(rawURL)
+ if err != nil {
+ return nil, err
+ }
+ protoPath := extractProtoPath(url.Path)
  config := clientConfig{
+ URL: url,
  Protocol: &protocolConnect{},
  Procedure: protoPath,
  CompressionPools: make(map[string]*compressionPool),
@@ -229,3 +238,19 @@ func (c *clientConfig) newSpec(t StreamType) Spec {
  IsClient: true,
  }
 }
+
+func parseRequestURL(rawURL string) (*url.URL, *Error) {
+ url, err := url.ParseRequestURI(rawURL)
+ if err == nil {
+ return url, nil
+ }
+ if !strings.Contains(rawURL, "://") {
+ // URL doesn't have a scheme, so the user is likely accustomed to
+ // grpc-go's APIs.
+ err = fmt.Errorf(
+ "URL %q missing scheme: use http:// or https:// (unlike grpc-go)",
+ rawURL,
+ )
+ }
+ return nil, NewError(CodeUnavailable, err)
+}

duplex_http_call.go

@@ -20,6 +20,7 @@ import (
  "fmt"
  "io"
  "net/http"
+ "net/url"
  "sync"
 )
 
@@ -52,19 +53,33 @@ type duplexHTTPCall struct {
 func newDuplexHTTPCall(
  ctx context.Context,
  httpClient HTTPClient,
- url string,
+ url *url.URL,
  spec Spec,
  header http.Header,
 ) *duplexHTTPCall {
+ // ensure we make a copy of the url before we pass along to the
+ // Request. This ensures if a transport out of our control wants
+ // to mutate the req.URL, we don't feel the effects of it.
+ url = cloneURL(url)
  pipeReader, pipeWriter := io.Pipe()
- request, err := http.NewRequestWithContext(
- ctx,
- http.MethodPost,
- url,
- pipeReader,
- )
- request.Header = header
- client := &duplexHTTPCall{
+
+ // This is mirroring what http.NewRequestContext did, but
+ // using an already parsed url.URL object, rather than a string
+ // and parsing it again. This is a bit funny with HTTP/1.1
+ // explicitly, but this is logic copied over from
+ // NewRequestContext and doesn't effect the actual version
+ // being transmitted.
+ request := (&http.Request{
+ Method: http.MethodPost,
+ URL: url,
+ Header: header,
+ Proto: "HTTP/1.1",
+ ProtoMajor: 1,
+ ProtoMinor: 1,
+ Body: pipeReader,
+ Host: url.Host,
+ }).WithContext(ctx)
+ return &duplexHTTPCall{
  ctx: ctx,
  httpClient: httpClient,
  streamType: spec.StreamType,
@@ -73,15 +88,6 @@ func newDuplexHTTPCall(
  request: request,
  responseReady: make(chan struct{}),
  }
- if err != nil {
- // We can't construct a request, so we definitely can't send it over the
- // network. Exhaust the sync.Once immediately and short-circuit Read and
- // Write by setting an error.
- client.sendRequestOnce.Do(func() {})
- connectErr := errorf(CodeUnavailable, "construct *http.Request: %w", err)
- client.SetError(connectErr)
- }
- return client
 }
 
 // Write to the request body. Returns an error wrapping io.EOF after SetError
@@ -276,3 +282,17 @@ func (d *duplexHTTPCall) getError() error {
  defer d.errMu.Unlock()
  return d.err
 }
+
+// See: https://cs.opensource.google/go/go/+/refs/tags/go1.20.1:src/net/http/clone.go;l=22-33
+func cloneURL(oldURL *url.URL) *url.URL {
+ if oldURL == nil {
+ return nil
+ }
+ newURL := new(url.URL)
+ *newURL = *oldURL
+ if oldURL.User != nil {
+ newURL.User = new(url.Userinfo)
+ *newURL.User = *oldURL.User
+ }
+ return newURL
+}

protobuf_util.go

@@ -22,8 +22,8 @@ import (
 // corresponding to the Protobuf package, service, and method. It always starts
 // with a slash. Within connect, we use this as (1) Spec.Procedure and (2) the
 // path when mounting handlers on muxes.
-func extractProtoPath(url string) string {
- segments := strings.Split(url, "/")
+func extractProtoPath(path string) string {
+ segments := strings.Split(path, "/")
  var pkg, method string
  if len(segments) > 0 {
  pkg = segments[0]

protocol.go

@@ -109,7 +109,7 @@ type protocolClientParams struct {
  Codec Codec
  CompressMinBytes int
  HTTPClient HTTPClient
- URL string
+ URL *url.URL
  BufferPool *bufferPool
  ReadMaxBytes int
  SendMaxBytes int
@@ -238,22 +238,6 @@ func discard(reader io.Reader) error {
  return err
 }
 
-func validateRequestURL(rawURL string) (*url.URL, *Error) {
- url, err := url.ParseRequestURI(rawURL)
- if err == nil {
- return url, nil
- }
- if !strings.Contains(rawURL, "://") {
- // URL doesn't have a scheme, so the user is likely accustomed to
- // grpc-go's APIs.
- err = fmt.Errorf(
- "URL %q missing scheme: use http:// or https:// (unlike grpc-go)",
- rawURL,
- )
- }
- return nil, NewError(CodeUnavailable, err)
-}
-
 // negotiateCompression determines and validates the request compression and
 // response compression using the available compressors and protocol-specific
 // Content-Encoding and Accept-Encoding headers.

protocol_connect.go

@@ -74,13 +74,9 @@ func (*protocolConnect) NewHandler(params *protocolHandlerParams) protocolHandle
 
 // NewClient implements protocol, so it must return an interface.
 func (*protocolConnect) NewClient(params *protocolClientParams) (protocolClient, error) {
- url, err := validateRequestURL(params.URL)
- if err != nil {
- return nil, err
- }
  return &connectClient{
  protocolClientParams: *params,
- peer: newPeerFromURL(url, ProtocolConnect),
+ peer: newPeerFromURL(params.URL, ProtocolConnect),
  }, nil
 }
 

protocol_grpc.go

@@ -110,13 +110,9 @@ func (g *protocolGRPC) NewHandler(params *protocolHandlerParams) protocolHandler
 
 // NewClient implements protocol, so it must return an interface.
 func (g *protocolGRPC) NewClient(params *protocolClientParams) (protocolClient, error) {
- url, err := validateRequestURL(params.URL)
- if err != nil {
- return nil, err
- }
- peer := newPeerFromURL(url, ProtocolGRPC)
+ peer := newPeerFromURL(params.URL, ProtocolGRPC)
  if g.web {
- peer = newPeerFromURL(url, ProtocolGRPCWeb)
+ peer = newPeerFromURL(params.URL, ProtocolGRPCWeb)
  }
  return &grpcClient{
  protocolClientParams: *params,