From ae8251b0aa946177877f61b45a96e90319dce1ff Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 30 Jan 2014 09:57:04 +0100 Subject: [PATCH] net/http: use a struct instead of a string in transport conn cache key The Transport's idle connection cache is keyed by a string, for pre-Go 1.0 reasons. Ever since Go has been able to use structs as map keys, there's been a TODO in the code to use structs instead of allocating strings. This change does that. Saves 3 allocatins and ~100 bytes of garbage per client request. But because string hashing is so fast these days (thanks, Keith), the performance is a wash: what we gain on GC and not allocating, we lose in slower hashing. (hashing structs of strings is slower than 1 string) This seems a bit faster usually, but I've also seen it be a bit slower. But at least it's how I've wanted it now, and it the allocation improvements are consistent. LGTM=adg R=adg CC=golang-codereviews https://golang.org/cl/58260043 --- src/pkg/net/http/export_test.go | 11 +++--- src/pkg/net/http/proxy_test.go | 4 +-- src/pkg/net/http/transport.go | 64 ++++++++++++++++++--------------- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/src/pkg/net/http/export_test.go b/src/pkg/net/http/export_test.go index 0494991bde..8074df5bbd 100644 --- a/src/pkg/net/http/export_test.go +++ b/src/pkg/net/http/export_test.go @@ -32,7 +32,7 @@ func (t *Transport) IdleConnKeysForTesting() (keys []string) { return } for key := range t.idleConn { - keys = append(keys, key) + keys = append(keys, key.String()) } return } @@ -43,11 +43,12 @@ func (t *Transport) IdleConnCountForTesting(cacheKey string) int { if t.idleConn == nil { return 0 } - conns, ok := t.idleConn[cacheKey] - if !ok { - return 0 + for k, conns := range t.idleConn { + if k.String() == cacheKey { + return len(conns) + } } - return len(conns) + return 0 } func (t *Transport) IdleConnChMapSizeForTesting() int { diff --git a/src/pkg/net/http/proxy_test.go b/src/pkg/net/http/proxy_test.go index 449ccaeea7..d0726f61f3 100644 --- a/src/pkg/net/http/proxy_test.go +++ b/src/pkg/net/http/proxy_test.go @@ -71,8 +71,8 @@ func TestCacheKeys(t *testing.T) { proxy = u } cm := connectMethod{proxy, tt.scheme, tt.addr} - if cm.String() != tt.key { - t.Fatalf("{%q, %q, %q} cache key %q; want %q", tt.proxy, tt.scheme, tt.addr, cm.String(), tt.key) + if got := cm.key().String(); got != tt.key { + t.Fatalf("{%q, %q, %q} cache key = %q; want %q", tt.proxy, tt.scheme, tt.addr, got, tt.key) } } } diff --git a/src/pkg/net/http/transport.go b/src/pkg/net/http/transport.go index 8fc7329e36..df17178235 100644 --- a/src/pkg/net/http/transport.go +++ b/src/pkg/net/http/transport.go @@ -41,8 +41,8 @@ const DefaultMaxIdleConnsPerHost = 2 // Transport can also cache connections for future re-use. type Transport struct { idleMu sync.Mutex - idleConn map[string][]*persistConn - idleConnCh map[string]chan *persistConn + idleConn map[connectMethodKey][]*persistConn + idleConnCh map[connectMethodKey]chan *persistConn reqMu sync.Mutex reqConn map[*Request]*persistConn altMu sync.RWMutex @@ -281,17 +281,11 @@ func (e *envOnce) reset() { e.val = "" } -func (t *Transport) connectMethodForRequest(treq *transportRequest) (*connectMethod, error) { - cm := &connectMethod{ - targetScheme: treq.URL.Scheme, - targetAddr: canonicalAddr(treq.URL), - } +func (t *Transport) connectMethodForRequest(treq *transportRequest) (cm connectMethod, err error) { + cm.targetScheme = treq.URL.Scheme + cm.targetAddr = canonicalAddr(treq.URL) if t.Proxy != nil { - var err error cm.proxyURL, err = t.Proxy(treq.Request) - if err != nil { - return nil, err - } } return cm, nil } @@ -347,7 +341,7 @@ func (t *Transport) putIdleConn(pconn *persistConn) bool { } } if t.idleConn == nil { - t.idleConn = make(map[string][]*persistConn) + t.idleConn = make(map[connectMethodKey][]*persistConn) } if len(t.idleConn[key]) >= max { t.idleMu.Unlock() @@ -367,7 +361,7 @@ func (t *Transport) putIdleConn(pconn *persistConn) bool { // getIdleConnCh returns a channel to receive and return idle // persistent connection for the given connectMethod. // It may return nil, if persistent connections are not being used. -func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn { +func (t *Transport) getIdleConnCh(cm connectMethod) chan *persistConn { if t.DisableKeepAlives { return nil } @@ -375,7 +369,7 @@ func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn { t.idleMu.Lock() defer t.idleMu.Unlock() if t.idleConnCh == nil { - t.idleConnCh = make(map[string]chan *persistConn) + t.idleConnCh = make(map[connectMethodKey]chan *persistConn) } ch, ok := t.idleConnCh[key] if !ok { @@ -385,7 +379,7 @@ func (t *Transport) getIdleConnCh(cm *connectMethod) chan *persistConn { return ch } -func (t *Transport) getIdleConn(cm *connectMethod) (pconn *persistConn) { +func (t *Transport) getIdleConn(cm connectMethod) (pconn *persistConn) { key := cm.key() t.idleMu.Lock() defer t.idleMu.Unlock() @@ -404,7 +398,7 @@ func (t *Transport) getIdleConn(cm *connectMethod) (pconn *persistConn) { // 2 or more cached connections; pop last // TODO: queue? pconn = pconns[len(pconns)-1] - t.idleConn[key] = pconns[0 : len(pconns)-1] + t.idleConn[key] = pconns[:len(pconns)-1] } if !pconn.isBroken() { return @@ -436,7 +430,7 @@ func (t *Transport) dial(network, addr string) (c net.Conn, err error) { // specified in the connectMethod. This includes doing a proxy CONNECT // and/or setting up TLS. If this doesn't return an error, the persistConn // is ready to write requests to. -func (t *Transport) getConn(cm *connectMethod) (*persistConn, error) { +func (t *Transport) getConn(cm connectMethod) (*persistConn, error) { if pc := t.getIdleConn(cm); pc != nil { return pc, nil } @@ -471,7 +465,7 @@ func (t *Transport) getConn(cm *connectMethod) (*persistConn, error) { } } -func (t *Transport) dialConn(cm *connectMethod) (*persistConn, error) { +func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) { conn, err := t.dial("tcp", cm.addr()) if err != nil { if cm.proxyURL != nil { @@ -634,20 +628,20 @@ type connectMethod struct { targetAddr string // Not used if proxy + http targetScheme (4th example in table) } -func (ck *connectMethod) key() string { - return ck.String() // TODO: use a struct type instead -} - -func (ck *connectMethod) String() string { +func (cm *connectMethod) key() connectMethodKey { proxyStr := "" - targetAddr := ck.targetAddr - if ck.proxyURL != nil { - proxyStr = ck.proxyURL.String() - if ck.targetScheme == "http" { + targetAddr := cm.targetAddr + if cm.proxyURL != nil { + proxyStr = cm.proxyURL.String() + if cm.targetScheme == "http" { targetAddr = "" } } - return strings.Join([]string{proxyStr, ck.targetScheme, targetAddr}, "|") + return connectMethodKey{ + proxy: proxyStr, + scheme: cm.targetScheme, + addr: targetAddr, + } } // addr returns the first hop "host:port" to which we need to TCP connect. @@ -668,11 +662,23 @@ func (cm *connectMethod) tlsHost() string { return h } +// connectMethodKey is the map key version of connectMethod, with a +// stringified proxy URL (or the empty string) instead of a pointer to +// a URL. +type connectMethodKey struct { + proxy, scheme, addr string +} + +func (k connectMethodKey) String() string { + // Only used by tests. + return fmt.Sprintf("%s|%s|%s", k.proxy, k.scheme, k.addr) +} + // persistConn wraps a connection, usually a persistent one // (but may be used for non-keep-alive requests as well) type persistConn struct { t *Transport - cacheKey string // its connectMethod.String() + cacheKey connectMethodKey conn net.Conn closed bool // whether conn has been closed br *bufio.Reader // from conn -- 2.48.1