Skip to content

Commit dc2d64b

Browse files
bradfitzbradfitz
committed
cmd/go: cache results of HTTP requests done during meta tag discovery
Previously, running $ go get -u -v golang.org/x/tools/cmd/godoc would results in dozens of HTTP requests for https://golang.org/x/tools?go-get=1 once per package under x/tools. Now it caches the results. We still end up doing one HTTP request for all the packages under x/tools, but this reduces the total number of HTTP requests in ~half. This also moves the singleflight package back into an internal package. singleflight was originally elsewhere as a package, then got copied into "net" (without its tests). But now that we have internal, put it in its own package, and restore its test. Fixes golang#9249 Change-Id: Ieb5cf04fc4d0a0c188cb957efdc7ea3068c34e3f Reviewed-on: https://go-review.googlesource.com/8727 Reviewed-by: Ian Lance Taylor <iant@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
1 parent d1af6be commit dc2d64b

File tree

6 files changed

+170
-41
lines changed

6 files changed

+170
-41
lines changed

‎src/cmd/dist/build.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -859,6 +859,7 @@ var buildorder = []string{
859859
"errors",
860860
"sync/atomic",
861861
"sync",
862+
"internal/singleflight",
862863
"io",
863864
"unicode",
864865
"unicode/utf8",

‎src/cmd/go/vcs.go‎

Lines changed: 71 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,14 @@ import (
99
"encoding/json"
1010
"errors"
1111
"fmt"
12+
"internal/singleflight"
1213
"log"
1314
"os"
1415
"os/exec"
1516
"path/filepath"
1617
"regexp"
1718
"strings"
19+
"sync"
1820
)
1921

2022
// A vcsCmd describes how to use a version control system
@@ -566,7 +568,7 @@ func repoRootForImportPathStatic(importPath, scheme string) (*repoRoot, error) {
566568
// repoRootForImportDynamic finds a *repoRoot for a custom domain that's not
567569
// statically known by repoRootForImportPathStatic.
568570
//
569-
// This handles "vanity import paths" like "name.tld/pkg/foo".
571+
// This handles custom import paths like "name.tld/pkg/foo".
570572
func repoRootForImportDynamic(importPath string) (*repoRoot, error) {
571573
slash := strings.Index(importPath, "/")
572574
if slash < 0 {
@@ -585,58 +587,106 @@ func repoRootForImportDynamic(importPath string) (*repoRoot, error) {
585587
if err != nil {
586588
return nil, fmt.Errorf("parsing %s: %v", importPath, err)
587589
}
588-
metaImport, err := matchGoImport(imports, importPath)
590+
// Find the matched meta import.
591+
mmi, err := matchGoImport(imports, importPath)
589592
if err != nil {
590593
if err != errNoMatch {
591594
return nil, fmt.Errorf("parse %s: %v", urlStr, err)
592595
}
593596
return nil, fmt.Errorf("parse %s: no go-import meta tags", urlStr)
594597
}
595598
if buildV {
596-
log.Printf("get %q: found meta tag %#v at %s", importPath, metaImport, urlStr)
599+
log.Printf("get %q: found meta tag %#v at %s", importPath, mmi, urlStr)
597600
}
598601
// If the import was "uni.edu/bob/project", which said the
599602
// prefix was "uni.edu" and the RepoRoot was "evilroot.com",
600603
// make sure we don't trust Bob and check out evilroot.com to
601604
// "uni.edu" yet (possibly overwriting/preempting another
602605
// non-evil student). Instead, first verify the root and see
603606
// if it matches Bob's claim.
604-
if metaImport.Prefix != importPath {
607+
if mmi.Prefix != importPath {
605608
if buildV {
606609
log.Printf("get %q: verifying non-authoritative meta tag", importPath)
607610
}
608611
urlStr0 := urlStr
609-
urlStr, body, err = httpsOrHTTP(metaImport.Prefix)
612+
var imports []metaImport
613+
urlStr, imports, err = metaImportsForPrefix(mmi.Prefix)
610614
if err != nil {
611-
return nil, fmt.Errorf("fetch %s: %v", urlStr, err)
612-
}
613-
imports, err := parseMetaGoImports(body)
614-
if err != nil {
615-
return nil, fmt.Errorf("parsing %s: %v", importPath, err)
616-
}
617-
if len(imports) == 0 {
618-
return nil, fmt.Errorf("fetch %s: no go-import meta tag", urlStr)
615+
return nil, err
619616
}
620617
metaImport2, err := matchGoImport(imports, importPath)
621-
if err != nil || metaImport != metaImport2 {
622-
return nil, fmt.Errorf("%s and %s disagree about go-import for %s", urlStr0, urlStr, metaImport.Prefix)
618+
if err != nil || mmi != metaImport2 {
619+
return nil, fmt.Errorf("%s and %s disagree about go-import for %s", urlStr0, urlStr, mmi.Prefix)
623620
}
624621
}
625622

626-
if !strings.Contains(metaImport.RepoRoot, "://") {
627-
return nil, fmt.Errorf("%s: invalid repo root %q; no scheme", urlStr, metaImport.RepoRoot)
623+
if !strings.Contains(mmi.RepoRoot, "://") {
624+
return nil, fmt.Errorf("%s: invalid repo root %q; no scheme", urlStr, mmi.RepoRoot)
628625
}
629626
rr := &repoRoot{
630-
vcs: vcsByCmd(metaImport.VCS),
631-
repo: metaImport.RepoRoot,
632-
root: metaImport.Prefix,
627+
vcs: vcsByCmd(mmi.VCS),
628+
repo: mmi.RepoRoot,
629+
root: mmi.Prefix,
633630
}
634631
if rr.vcs == nil {
635-
return nil, fmt.Errorf("%s: unknown vcs %q", urlStr, metaImport.VCS)
632+
return nil, fmt.Errorf("%s: unknown vcs %q", urlStr, mmi.VCS)
636633
}
637634
return rr, nil
638635
}
639636

637+
var fetchGroup singleflight.Group
638+
var (
639+
fetchCacheMu sync.Mutex
640+
fetchCache = map[string]fetchResult{} // key is metaImportsForPrefix's importPrefix
641+
)
642+
643+
// metaImportsForPrefix takes a package's root import path as declared in a <meta> tag
644+
// and returns its HTML discovery URL and the parsed metaImport lines
645+
// found on the page.
646+
//
647+
// The importPath is of the form "golang.org/x/tools".
648+
// It is an error if no imports are found.
649+
// urlStr will still be valid if err != nil.
650+
// The returned urlStr will be of the form "https://golang.org/x/tools?go-get=1"
651+
func metaImportsForPrefix(importPrefix string) (urlStr string, imports []metaImport, err error) {
652+
setCache := func(res fetchResult) (fetchResult, error) {
653+
fetchCacheMu.Lock()
654+
defer fetchCacheMu.Unlock()
655+
fetchCache[importPrefix] = res
656+
return res, nil
657+
}
658+
659+
resi, _, _ := fetchGroup.Do(importPrefix, func() (resi interface{}, err error) {
660+
fetchCacheMu.Lock()
661+
if res, ok := fetchCache[importPrefix]; ok {
662+
fetchCacheMu.Unlock()
663+
return res, nil
664+
}
665+
fetchCacheMu.Unlock()
666+
667+
urlStr, body, err := httpsOrHTTP(importPrefix)
668+
if err != nil {
669+
return setCache(fetchResult{urlStr: urlStr, err: fmt.Errorf("fetch %s: %v", urlStr, err)})
670+
}
671+
imports, err := parseMetaGoImports(body)
672+
if err != nil {
673+
return setCache(fetchResult{urlStr: urlStr, err: fmt.Errorf("parsing %s: %v", urlStr, err)})
674+
}
675+
if len(imports) == 0 {
676+
err = fmt.Errorf("fetch %s: no go-import meta tag", urlStr)
677+
}
678+
return setCache(fetchResult{urlStr: urlStr, imports: imports, err: err})
679+
})
680+
res := resi.(fetchResult)
681+
return res.urlStr, res.imports, res.err
682+
}
683+
684+
type fetchResult struct {
685+
urlStr string // e.g. "https://foo.com/x/bar?go-get=1"
686+
imports []metaImport
687+
err error
688+
}
689+
640690
// metaImport represents the parsed <meta name="go-import"
641691
// content="prefix vcs reporoot" /> tags from HTML files.
642692
type metaImport struct {

‎src/go/build/deps_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ var pkgDeps = map[string][]string{
240240
// Basic networking.
241241
// Because net must be used by any package that wants to
242242
// do networking portably, it must have a small dependency set: just L1+basic os.
243-
"net": {"L1", "CGO", "os", "syscall", "time", "internal/syscall/windows"},
243+
"net": {"L1", "CGO", "os", "syscall", "time", "internal/syscall/windows", "internal/singleflight"},
244244

245245
// NET enables use of basic network-related packages.
246246
"NET": {

src/net/singleflight.go renamed to src/internal/singleflight/singleflight.go

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
// Use of this source code is governed by a BSD-style
33
// license that can be found in the LICENSE file.
44

5-
package net
5+
// Package singleflight provides a duplicate function call suppression
6+
// mechanism.
7+
package singleflight
68

79
import "sync"
810

@@ -19,30 +21,30 @@ type call struct {
1921
// mutex held before the WaitGroup is done, and are read but
2022
// not written after the WaitGroup is done.
2123
dups int
22-
chans []chan<- singleflightResult
24+
chans []chan<- Result
2325
}
2426

25-
// singleflight represents a class of work and forms a namespace in
27+
// Group represents a class of work and forms a namespace in
2628
// which units of work can be executed with duplicate suppression.
27-
type singleflight struct {
29+
type Group struct {
2830
mu sync.Mutex // protects m
2931
m map[string]*call // lazily initialized
3032
}
3133

32-
// singleflightResult holds the results of Do, so they can be passed
34+
// Result holds the results of Do, so they can be passed
3335
// on a channel.
34-
type singleflightResult struct {
35-
v interface{}
36-
err error
37-
shared bool
36+
type Result struct {
37+
Val interface{}
38+
Err error
39+
Shared bool
3840
}
3941

4042
// Do executes and returns the results of the given function, making
4143
// sure that only one execution is in-flight for a given key at a
4244
// time. If a duplicate comes in, the duplicate caller waits for the
4345
// original to complete and receives the same results.
4446
// The return value shared indicates whether v was given to multiple callers.
45-
func (g *singleflight) Do(key string, fn func() (interface{}, error)) (v interface{}, err error, shared bool) {
47+
func (g *Group) Do(key string, fn func() (interface{}, error)) (v interface{}, err error, shared bool) {
4648
g.mu.Lock()
4749
if g.m == nil {
4850
g.m = make(map[string]*call)
@@ -64,8 +66,8 @@ func (g *singleflight) Do(key string, fn func() (interface{}, error)) (v interfa
6466

6567
// DoChan is like Do but returns a channel that will receive the
6668
// results when they are ready.
67-
func (g *singleflight) DoChan(key string, fn func() (interface{}, error)) <-chan singleflightResult {
68-
ch := make(chan singleflightResult, 1)
69+
func (g *Group) DoChan(key string, fn func() (interface{}, error)) <-chan Result {
70+
ch := make(chan Result, 1)
6971
g.mu.Lock()
7072
if g.m == nil {
7173
g.m = make(map[string]*call)
@@ -76,7 +78,7 @@ func (g *singleflight) DoChan(key string, fn func() (interface{}, error)) <-chan
7678
g.mu.Unlock()
7779
return ch
7880
}
79-
c := &call{chans: []chan<- singleflightResult{ch}}
81+
c := &call{chans: []chan<- Result{ch}}
8082
c.wg.Add(1)
8183
g.m[key] = c
8284
g.mu.Unlock()
@@ -87,22 +89,22 @@ func (g *singleflight) DoChan(key string, fn func() (interface{}, error)) <-chan
8789
}
8890

8991
// doCall handles the single call for a key.
90-
func (g *singleflight) doCall(c *call, key string, fn func() (interface{}, error)) {
92+
func (g *Group) doCall(c *call, key string, fn func() (interface{}, error)) {
9193
c.val, c.err = fn()
9294
c.wg.Done()
9395

9496
g.mu.Lock()
9597
delete(g.m, key)
9698
for _, ch := range c.chans {
97-
ch <- singleflightResult{c.val, c.err, c.dups > 0}
99+
ch <- Result{c.val, c.err, c.dups > 0}
98100
}
99101
g.mu.Unlock()
100102
}
101103

102104
// Forget tells the singleflight to forget about a key. Future calls
103105
// to Do for this key will call the function rather than waiting for
104106
// an earlier call to complete.
105-
func (g *singleflight) Forget(key string) {
107+
func (g *Group) Forget(key string) {
106108
g.mu.Lock()
107109
delete(g.m, key)
108110
g.mu.Unlock()

‎src/internal/singleflight/singleflight_test.go‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// Copyright 2013 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package singleflight
6+
7+
import (
8+
"errors"
9+
"fmt"
10+
"sync"
11+
"sync/atomic"
12+
"testing"
13+
"time"
14+
)
15+
16+
func TestDo(t *testing.T) {
17+
var g Group
18+
v, err, _ := g.Do("key", func() (interface{}, error) {
19+
return "bar", nil
20+
})
21+
if got, want := fmt.Sprintf("%v (%T)", v, v), "bar (string)"; got != want {
22+
t.Errorf("Do = %v; want %v", got, want)
23+
}
24+
if err != nil {
25+
t.Errorf("Do error = %v", err)
26+
}
27+
}
28+
29+
func TestDoErr(t *testing.T) {
30+
var g Group
31+
someErr := errors.New("Some error")
32+
v, err, _ := g.Do("key", func() (interface{}, error) {
33+
return nil, someErr
34+
})
35+
if err != someErr {
36+
t.Errorf("Do error = %v; want someErr %v", err, someErr)
37+
}
38+
if v != nil {
39+
t.Errorf("unexpected non-nil value %#v", v)
40+
}
41+
}
42+
43+
func TestDoDupSuppress(t *testing.T) {
44+
var g Group
45+
c := make(chan string)
46+
var calls int32
47+
fn := func() (interface{}, error) {
48+
atomic.AddInt32(&calls, 1)
49+
return <-c, nil
50+
}
51+
52+
const n = 10
53+
var wg sync.WaitGroup
54+
for i := 0; i < n; i++ {
55+
wg.Add(1)
56+
go func() {
57+
v, err, _ := g.Do("key", fn)
58+
if err != nil {
59+
t.Errorf("Do error: %v", err)
60+
}
61+
if v.(string) != "bar" {
62+
t.Errorf("got %q; want %q", v, "bar")
63+
}
64+
wg.Done()
65+
}()
66+
}
67+
time.Sleep(100 * time.Millisecond) // let goroutines above block
68+
c <- "bar"
69+
wg.Wait()
70+
if got := atomic.LoadInt32(&calls); got != 1 {
71+
t.Errorf("number of calls = %d; want 1", got)
72+
}
73+
}

‎src/net/lookup.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44

55
package net
66

7-
import "time"
7+
import (
8+
"internal/singleflight"
9+
"time"
10+
)
811

912
// protocols contains minimal mappings between internet protocol
1013
// names and numbers for platforms that don't have a complete list of
@@ -39,7 +42,7 @@ func LookupIP(host string) (ips []IP, err error) {
3942
return
4043
}
4144

42-
var lookupGroup singleflight
45+
var lookupGroup singleflight.Group
4346

4447
// lookupIPMerge wraps lookupIP, but makes sure that for any given
4548
// host, only one lookup is in-flight at a time. The returned memory
@@ -98,7 +101,7 @@ func lookupIPDeadline(host string, deadline time.Time) (addrs []IPAddr, err erro
98101
return nil, errTimeout
99102

100103
case r := <-ch:
101-
return lookupIPReturn(r.v, r.err, r.shared)
104+
return lookupIPReturn(r.Val, r.Err, r.Shared)
102105
}
103106
}
104107

0 commit comments

Comments
 (0)