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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by rsc
Modified:
10 years, 6 months 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/
10 years, 7 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 ...
10 years, 7 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> > ...
10 years, 7 months ago (2013-08-16 05:32:21 UTC) #3
r
LGTM
10 years, 7 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++ { ...
10 years, 7 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> ...
10 years, 7 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, ...
10 years, 7 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 ...
10 years, 7 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 ...
10 years, 7 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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> ...
10 years, 6 months ago (2013-09-23 21:05:13 UTC) #15
rsc
> If you have 2 unordered by HB memory accesses, you have a data race. ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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. ...
10 years, 6 months 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 ...
10 years, 6 months 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, ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months 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 ...
10 years, 6 months ago (2013-09-24 05:30:12 UTC) #28
kortschak
Thanks. Dan
10 years, 6 months 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 ...
10 years, 6 months ago (2013-09-24 05:49:12 UTC) #30
rsc
10 years, 6 months 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