Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(288)

Issue 13038043: code review 13038043: net: limit number of concurrent cgo calls (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 2 months ago by rsc
Modified:
11 years ago
Reviewers:
r, kortschak, dvyukov
CC:
golang-dev, r, bradfitz
Visibility:
Public.

Description

net: limit number of concurrent cgo calls The limit is 500. There is no way to change it. This primarily affects name resolution. If a million goroutines try to resolve DNS names, only 500 will get to execute cgo calls at a time. But in return the operating system will not crash. Fixes issue 5625.

Patch Set 1 #

Patch Set 2 : diff -r aeb72c90c10e https://code.google.com/p/go/ #

Patch Set 3 : diff -r aeb72c90c10e https://code.google.com/p/go/ #

Total comments: 3

Patch Set 4 : diff -r b3ee0be87a1c https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -0 lines) Patch
M src/pkg/net/cgo_unix.go View 1 2 chunks +6 lines, -0 lines 0 comments Download
M src/pkg/net/dialgoogle_test.go View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
M src/pkg/net/lookup_windows.go View 1 13 chunks +25 lines, -0 lines 0 comments Download
M src/pkg/net/net.go View 1 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 31
rsc
Hello golang-dev@googlegroups.com (cc: bradfitz), I'd like you to review this change to https://code.google.com/p/go/
11 years, 2 months ago (2013-08-16 03:15:47 UTC) #1
kortschak
https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go File src/pkg/net/net.go (right): https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446 src/pkg/net/net.go:446: threadLimit <- struct{}{} Is this safe since threadLimit is ...
11 years, 2 months ago (2013-08-16 03:34:15 UTC) #2
rsc
On Thu, Aug 15, 2013 at 11:34 PM, <dan.kortschak@adelaide.edu.au> wrote: > https://codereview.appspot.**com/13038043/diff/6001/src/** > pkg/net/net.go#newcode446<https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/net.go#newcode446> > ...
11 years, 2 months ago (2013-08-16 05:32:21 UTC) #3
r
LGTM
11 years, 2 months ago (2013-08-16 05:38:49 UTC) #4
dvyukov
https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/dialgoogle_test.go File src/pkg/net/dialgoogle_test.go (right): https://codereview.appspot.com/13038043/diff/6001/src/pkg/net/dialgoogle_test.go#newcode71 src/pkg/net/dialgoogle_test.go:71: for i := 0; i < N*9/10; i++ { ...
11 years, 2 months ago (2013-08-16 08:26:52 UTC) #5
dvyukov
On 2013/08/16 05:32:21, rsc wrote: > On Thu, Aug 15, 2013 at 11:34 PM, <mailto:dan.kortschak@adelaide.edu.au> ...
11 years, 2 months ago (2013-08-16 08:33:11 UTC) #6
rsc
On 2013/08/16 08:33:11, dvyukov wrote: > On 2013/08/16 05:32:21, rsc wrote: > > On Thu, ...
11 years, 2 months ago (2013-08-17 01:24:10 UTC) #7
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=565e2d46bb68 *** net: limit number of concurrent cgo calls The limit is ...
11 years, 2 months ago (2013-08-17 02:43:08 UTC) #8
dvyukov
On 2013/08/17 01:24:10, rsc wrote: > On 2013/08/16 08:33:11, dvyukov wrote: > > On 2013/08/16 ...
11 years, 2 months ago (2013-08-17 09:30:58 UTC) #9
rsc
On Sat, Aug 17, 2013 at 5:30 AM, <dvyukov@google.com> wrote: > Memory model is relevant ...
11 years, 1 month ago (2013-09-06 19:30:56 UTC) #10
kortschak
On 2013/09/06 19:30:56, rsc wrote: > It sounds like you are claiming there is no ...
11 years, 1 month ago (2013-09-06 20:46:43 UTC) #11
r
I'm going to support Russ here. We don't care about the ordering so avoiding the ...
11 years, 1 month ago (2013-09-09 00:20:47 UTC) #12
kortschak
Personally, I'm happy that the original approach is being used - it's clearer and more ...
11 years ago (2013-09-19 01:00:42 UTC) #13
rsc
On Wed, Sep 18, 2013 at 9:00 PM, <dan.kortschak@adelaide.edu.au> wrote: > The use of it ...
11 years ago (2013-09-23 17:39:46 UTC) #14
dvyukov
On 2013/09/23 17:39:46, rsc wrote: > On Wed, Sep 18, 2013 at 9:00 PM, <mailto:dan.kortschak@adelaide.edu.au> ...
11 years ago (2013-09-23 21:05:13 UTC) #15
rsc
> If you have 2 unordered by HB memory accesses, you have a data race. ...
11 years ago (2013-09-23 21:14:11 UTC) #16
dvyukov
On 2013/09/23 21:14:11, rsc wrote: > > If you have 2 unordered by HB memory ...
11 years ago (2013-09-23 21:22:35 UTC) #17
kortschak
The issue for me is if the memory model is not ordering things then what ...
11 years ago (2013-09-23 22:03:10 UTC) #18
dvyukov
On Mon, Sep 23, 2013 at 3:03 PM, Dan Kortschak < dan.kortschak@adelaide.edu.au> wrote: > The ...
11 years ago (2013-09-23 23:36:45 UTC) #19
kortschak
On Mon, 2013-09-23 at 16:36 -0700, Dmitry Vyukov wrote: > Addressing that single case is ...
11 years ago (2013-09-23 23:49:33 UTC) #20
rsc
The code before my CL had an unlimited number of concurrent calls to a function ...
11 years ago (2013-09-24 02:32:24 UTC) #21
kortschak
I think this thread is unfortunately tangled, and I apologise if I contributed to that. ...
11 years ago (2013-09-24 03:01:14 UTC) #22
rsc
On Mon, Sep 23, 2013 at 11:01 PM, Dan Kortschak < dan.kortschak@adelaide.edu.au> wrote: > The ...
11 years ago (2013-09-24 03:14:30 UTC) #23
dvyukov
On Mon, Sep 23, 2013 at 8:14 PM, Russ Cox <rsc@golang.org> wrote: > On Mon, ...
11 years ago (2013-09-24 04:23:11 UTC) #24
rsc
If you have a channel of capacity 500, you cannot execute 501 sends before any ...
11 years ago (2013-09-24 05:03:05 UTC) #25
dvyukov
On Mon, Sep 23, 2013 at 10:03 PM, Russ Cox <rsc@golang.org> wrote: > If you ...
11 years ago (2013-09-24 05:11:56 UTC) #26
kortschak
So what does this do to the hypothetical optimisation path that Dmitry sketched in his ...
11 years ago (2013-09-24 05:14:58 UTC) #27
rsc
The rewrite Dmitriy suggested is not really something an optimizing compiler could ever do except ...
11 years ago (2013-09-24 05:30:12 UTC) #28
kortschak
Thanks. Dan
11 years ago (2013-09-24 05:33:05 UTC) #29
dvyukov
On Mon, Sep 23, 2013 at 10:30 PM, Russ Cox <rsc@golang.org> wrote: > The rewrite ...
11 years ago (2013-09-24 05:49:12 UTC) #30
rsc
11 years ago (2013-09-24 13:45:19 UTC) #31
On Tue, Sep 24, 2013 at 1:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Mon, Sep 23, 2013 at 10:30 PM, Russ Cox <rsc@golang.org> wrote:I agree
> that it's quite unlikely (probably) to happen in reality.
>
> But to make it clear, we agree that formally it is a bug (it does not
> limit concurrency), but we are relying on our belief that compilers
> are not smart enough and do not do any kind of global program analysis
> (something that modern compiler can actually do) and do not have any a
> priory knowledge about libc functions (which they actually do have
> today).
>

My point was only that even the memory model cannot justify an optimization
that would break this code. If there is a bug, I think the bug is in the
use of the memory model to justify hypothetical statement reordering. There
are other concerns besides memory access, channel communication being the
biggest one. I don't believe the compiler should be reordering across
channel operations, and of course it does not. If we need to add a sentence
about that to the memory model to make people happy, so be it.

Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b