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

Issue 12183044: code review 12183044: syscall: disable cpu profiling around fork (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by dvyukov
Modified:
10 years, 7 months ago
Reviewers:
dfc, bradfitz
CC:
golang-dev, bradfitz
Visibility:
Public.

Description

syscall: disable cpu profiling around fork Fixes issue 5517. Fixes issue 5659.

Patch Set 1 #

Patch Set 2 : diff -r 5333e97ad270 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 3 : diff -r 5333e97ad270 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 4 : diff -r 5333e97ad270 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 5 : diff -r 23e06fccd3b9 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 6 : diff -r 23e06fccd3b9 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 7 : diff -r 23e06fccd3b9 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 4

Patch Set 8 : diff -r 03ca8d80b840 https://dvyukov%40google.com@code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -4 lines) Patch
M src/pkg/runtime/pprof/pprof_test.go View 1 2 3 4 5 6 7 2 chunks +30 lines, -3 lines 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 5 6 7 3 chunks +30 lines, -1 line 0 comments Download
M src/pkg/syscall/exec_bsd.go View 1 2 3 4 3 chunks +7 lines, -0 lines 0 comments Download
M src/pkg/syscall/exec_linux.go View 1 2 chunks +7 lines, -0 lines 0 comments Download

Messages

Total messages: 8
dvyukov
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
10 years, 7 months ago (2013-08-08 17:53:39 UTC) #1
dvyukov
On 2013/08/08 17:53:39, dvyukov wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review ...
10 years, 7 months ago (2013-08-12 08:44:11 UTC) #2
bradfitz
LGTM https://codereview.appspot.com/12183044/diff/18001/src/pkg/runtime/pprof/pprof_test.go File src/pkg/runtime/pprof/pprof_test.go (right): https://codereview.appspot.com/12183044/diff/18001/src/pkg/runtime/pprof/pprof_test.go#newcode149 src/pkg/runtime/pprof/pprof_test.go:149: // This makes fork slower. does this reliably ...
10 years, 7 months ago (2013-08-12 22:58:09 UTC) #3
dvyukov
https://codereview.appspot.com/12183044/diff/18001/src/pkg/runtime/pprof/pprof_test.go File src/pkg/runtime/pprof/pprof_test.go (right): https://codereview.appspot.com/12183044/diff/18001/src/pkg/runtime/pprof/pprof_test.go#newcode149 src/pkg/runtime/pprof/pprof_test.go:149: // This makes fork slower. On 2013/08/12 22:58:09, bradfitz ...
10 years, 7 months ago (2013-08-13 09:01:15 UTC) #4
dvyukov
*** Submitted as https://code.google.com/p/go/source/detail?r=9eb1dd061b1f *** syscall: disable cpu profiling around fork Fixes issue 5517. Fixes ...
10 years, 7 months ago (2013-08-13 09:01:42 UTC) #5
dfc
On 2013/08/13 09:01:42, dvyukov wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=9eb1dd061b1f *** > > syscall: disable ...
10 years, 7 months ago (2013-08-21 02:32:52 UTC) #6
dvyukov
On 2013/08/21 02:32:52, dfc wrote: > On 2013/08/13 09:01:42, dvyukov wrote: > > *** Submitted ...
10 years, 7 months ago (2013-08-21 09:45:19 UTC) #7
dfc
10 years, 7 months ago (2013-08-21 10:03:49 UTC) #8
I understand, thank you for addressing my concerns.

On Wed, Aug 21, 2013 at 7:45 PM,  <dvyukov@google.com> wrote:
> On 2013/08/21 02:32:52, dfc wrote:
>>
>> On 2013/08/13 09:01:42, dvyukov wrote:
>> > *** Submitted as
>
> https://code.google.com/p/go/source/detail?r=9eb1dd061b1f ***
>>
>> >
>> > syscall: disable cpu profiling around fork
>> > Fixes issue 5517.
>> > Fixes issue 5659.
>> >
>> > R=golang-dev, bradfitz
>> > CC=golang-dev
>> > https://codereview.appspot.com/12183044
>
>
>> Dmitry, did adding an extra call to runtime_{Before,After}fork violate
>
> the
>>
>> contract at the top of the exec function ? Those functions aren't
>
> #pragma 7 so
>>
>> could cause a stack split and an allocation. Is this a real concern,
>
> or am I
>>
>> barking at shadows ?
>
>
> The special concerns are only applicable to child process (because it
> can be forked off with runtime mutex locked or something),
> runtime_{Before,After}fork must be called only in parent (which is a
> completely normal Go process with normal rules).
>
>
>
> https://codereview.appspot.com/12183044/
Sign in to reply to this message.

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