code review 10079043: net: coalesce duplicate in-flight DNS lookups

net: coalesce duplicate in-flight DNS lookups

In Issue 5625, Russ says: "We should at least have a cache of
inflight lookups, so that 100 simultaneous dials of one host
name don't do the work 100x. That's easy and (assume we forget
the answer once they all get it) doesn't pose any consistency
problems. It just merges simultaneous work."

This brings in singleflight (unexported) from Google /
Camlistore, but without its tests. Maybe we should put it
somewhere in the standard library. But not now.

Update issue 5625

[bradfitz, 2013-06-06 13:36:08]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[iant, 2013-06-06 22:42:40]

LGTM

In CL description, s/coallesce/coalesce/.

[cespare, 2013-06-07 08:17:59]

https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go
File src/pkg/net/lookup.go (right):

https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go#newco...
src/pkg/net/lookup.go:16: return addrs, err
return lookupHost(host)   ?

[rsc, 2013-06-10 17:47:00]

I agree with Ian: please return separate copies of the value if there were
multiple people waiting for it.

[bradfitz, 2013-06-10 18:24:49]

I believe that was Dominik's position, but I agree.

Will do.



On Mon, Jun 10, 2013 at 7:47 PM, <rsc@golang.org> wrote:

> I agree with Ian: please return separate copies of the value if there
> were multiple people waiting for it.
>
>
>
https://codereview.appspot.**com/10079043/<https://codereview.appspot.com/100...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dave_cheney.net, 2013-06-11 11:19:12]

I like this, singleflight is very nice. I have a concern that the singleflight
cache will cache lookups forever, although the libc resolver is possibly never
caching them. I don't think either is the correct behaviour.

[dave_cheney.net, 2013-06-11 11:19:32]

https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go
File src/pkg/net/lookup.go (right):

https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go#newco...
src/pkg/net/lookup.go:12: 
lookupHostMerge doesn't mean much to me, how about cachedLookupHost or
lookupHostCached? Or maybe just a comment to outline the role of this method.

[bradfitz, 2013-06-11 11:36:15]

On Tue, Jun 11, 2013 at 1:19 PM, <dave@cheney.net> wrote:

> I like this, singleflight is very nice. I have a concern that the
> singleflight cache will cache lookups forever,


It's nice that you like it, but I don't think you understand it.

It doesn't cache forever.

If nothing is in-flight, its map is empty.

[rog, 2013-06-11 11:40:49]

https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/singleflight.go
File src/pkg/net/singleflight.go (right):

https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/singleflight.go...
src/pkg/net/singleflight.go:27: func (g *singleflight) Do(key string, fn func()
(interface{}, error)) (interface{}, error) {
Since this is used in only one way, I wonder if it might be worth specialising 
interface{} to []string to save an allocation.

If the key was an argument to the function, we could lose the closure allocation
too.

[bradfitz, 2013-06-13 22:30:16]

Hello golang-dev@googlegroups.com, iant@golang.org, cespare@gmail.com,
rsc@golang.org, dave@cheney.net, rogpeppe@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[cespare, 2013-06-13 22:39:15]

https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go
File src/pkg/net/singleflight.go (right):

https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.g...
src/pkg/net/singleflight.go:14: dups int
gofmt

[remyoudompheng, 2013-06-13 22:47:00]

https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go
File src/pkg/net/singleflight.go (right):

https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.g...
src/pkg/net/singleflight.go:49: if c, ok := g.m[key]; ok {
I don't understand this. shared should just be (c.dups > 0), the map lookup is
unnecessary and confusing

[iant, 2013-06-13 22:47:09]

LGTM

[rsc, 2013-06-14 13:21:20]

LGTM with fix

https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go
File src/pkg/net/singleflight.go (right):

https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.g...
src/pkg/net/singleflight.go:49: if c, ok := g.m[key]; ok {
On 2013/06/13 22:47:05, remyoudompheng wrote:
> I don't understand this. shared should just be (c.dups > 0), the map lookup is
> unnecessary and confusing

I don't understand this either. I think what you really mean here is:

g.mu.Lock()
delete(g.m, key)
g.mu.Unlock()

return c.val, c.err, c.dups > 0

[bradfitz, 2013-06-14 15:59:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=e255db359405 ***

net: coalesce duplicate in-flight DNS lookups

In Issue 5625, Russ says: "We should at least have a cache of
inflight lookups, so that 100 simultaneous dials of one host
name don't do the work 100x. That's easy and (assume we forget
the answer once they all get it) doesn't pose any consistency
problems. It just merges simultaneous work."

This brings in singleflight (unexported) from Google /
Camlistore, but without its tests. Maybe we should put it
somewhere in the standard library. But not now.

Update issue 5625

R=golang-dev, iant, cespare, rsc, dave, rogpeppe, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/10079043