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

Issue 4960057: code review 4960057: runtime: implement pprof support for windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years ago by hector
Modified:
9 years ago
Reviewers:
CC:
brainman, jp, rsc, dvyukov, golang-dev
Visibility:
Public.

Description

runtime: implement pprof support for windows Credit to jp for proof of concept.

Patch Set 1 #

Patch Set 2 : diff -r b7badceba5ec https://go.googlecode.com/hg/ #

Total comments: 23

Patch Set 3 : diff -r 2d8706ce68b4 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 4 : diff -r 785d9a614916 https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 5 : diff -r 805742b53bf3 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 6 : diff -r c588e886a8bd https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r b80728111ff9 https://go.googlecode.com/hg/ #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+192 lines, -34 lines) Patch
M src/pkg/runtime/runtime.h View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
M src/pkg/runtime/windows/386/defs.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M src/pkg/runtime/windows/386/signal.c View 1 1 chunk +2 lines, -4 lines 0 comments Download
M src/pkg/runtime/windows/386/sys.s View 1 2 3 4 5 6 2 chunks +31 lines, -12 lines 0 comments Download
M src/pkg/runtime/windows/amd64/defs.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M src/pkg/runtime/windows/amd64/signal.c View 1 1 chunk +2 lines, -4 lines 0 comments Download
M src/pkg/runtime/windows/amd64/sys.s View 1 2 3 4 5 6 1 chunk +30 lines, -10 lines 0 comments Download
M src/pkg/runtime/windows/defs.c View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M src/pkg/runtime/windows/os.h View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M src/pkg/runtime/windows/thread.c View 1 2 3 4 5 6 4 chunks +108 lines, -2 lines 1 comment Download

Messages

Total messages: 47
hector
Hello alex.brainman@gmail.com, jp@webmaster.ms, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
9 years ago (2011-09-07 00:10:29 UTC) #1
jp
http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/386/sys.s File src/pkg/runtime/windows/386/sys.s (right): http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/386/sys.s#newcode102 src/pkg/runtime/windows/386/sys.s:102: CALL runtime·externalthreadhandler(SB) PUSHL $runtime·ctrlhandler1(SB) JMP runtime·externalthreadhandler(SB) ? http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/amd64/sys.s File ...
9 years ago (2011-09-07 00:26:24 UTC) #2
brainman
Nice and simple. As always <g>. Please, add "Fixes issue 2041" to CL description. http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/386/sys.s ...
9 years ago (2011-09-07 07:04:22 UTC) #3
jp
http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/thread.c#newcode61 src/pkg/runtime/windows/thread.c:61: runtime·stdcall(runtime·DuplicateHandle, 7, (uintptr)-1, (uintptr)-2, (uintptr)-1, &m->thandle, (uintptr)0, (uintptr)0, (uintptr)2); ...
9 years ago (2011-09-07 07:20:18 UTC) #4
brainman
On 2011/09/07 07:20:18, jp wrote: > > GetCurrentThread() always returns (HANDLE)(-2). > It is a ...
9 years ago (2011-09-07 07:30:49 UTC) #5
jp
http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/thread.c#newcode374 src/pkg/runtime/windows/thread.c:374: static void *timer; Maybe runtime·lock() or another mutex to ...
9 years ago (2011-09-07 07:34:41 UTC) #6
jp
On 2011/09/07 07:30:49, brainman wrote: > But I didn't see anyone aligning their data in ...
9 years ago (2011-09-07 07:39:15 UTC) #7
jp
http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/amd64/sys.s File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/amd64/sys.s#newcode137 src/pkg/runtime/windows/amd64/sys.s:137: PUSHQ 32(BP) What is suppose to be in 32(BP) ...
9 years ago (2011-09-07 07:50:58 UTC) #8
rsc
http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/386/sys.s File src/pkg/runtime/windows/386/sys.s (right): http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/386/sys.s#newcode121 src/pkg/runtime/windows/386/sys.s:121: SUBL $512, SP // space for M I suggest ...
9 years ago (2011-09-07 18:28:36 UTC) #9
hector
PTAL: I've changed it so that a timer is run for each M, due to ...
9 years ago (2011-09-07 22:40:23 UTC) #10
brainman
LGTM. Nice.
9 years ago (2011-09-08 00:09:06 UTC) #11
jp
http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/amd64/sys.s File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/4960057/diff/4/src/pkg/runtime/windows/amd64/sys.s#newcode107 src/pkg/runtime/windows/amd64/sys.s:107: RET On 2011/09/07 22:40:23, hector wrote: > On 2011/09/07 ...
9 years ago (2011-09-08 07:24:15 UTC) #12
hector
Can we hold off submitting this until I check that WT_EXECUTEINPERSISTENTTHREAD is not a better ...
9 years ago (2011-09-08 10:40:20 UTC) #13
hector
Ok I've uploaded a new diff, hopefully this'll be good enough. PTAL On 8 September ...
9 years ago (2011-09-08 22:54:07 UTC) #14
brainman
On 2011/09/08 22:54:07, hector wrote: > Ok I've uploaded a new diff, hopefully this'll be ...
9 years ago (2011-09-09 01:19:38 UTC) #15
hector
On Sep 9, 2011 2:19 AM, <alex.brainman@gmail.com> wrote: > > On 2011/09/08 22:54:07, hector wrote: ...
9 years ago (2011-09-09 07:36:59 UTC) #16
brainman
LGTM. Anyone else wants to comment?
9 years ago (2011-09-09 07:40:57 UTC) #17
dvyukov
http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c#newcode361 src/pkg/runtime/windows/thread.c:361: if((uintptr)runtime·stdcall(runtime·SuspendThread, 1, m->hthread) == -1) The return type is ...
9 years ago (2011-09-09 10:52:19 UTC) #18
dvyukov
http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c#newcode360 src/pkg/runtime/windows/thread.c:360: runtime·lock(&m->proflock); Why we need this lock?
9 years ago (2011-09-09 10:57:30 UTC) #19
hector
http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c#newcode360 src/pkg/runtime/windows/thread.c:360: runtime·lock(&m->proflock); On 2011/09/09 10:57:30, dvyukov wrote: > Why we ...
9 years ago (2011-09-09 11:08:56 UTC) #20
dvyukov
On 2011/09/07 22:40:23, hector wrote: > PTAL: > > I've changed it so that a ...
9 years ago (2011-09-09 11:15:02 UTC) #21
dvyukov
On 2011/09/09 11:08:56, hector wrote: > http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c > File src/pkg/runtime/windows/thread.c (right): > > http://codereview.appspot.com/4960057/diff/24001/src/pkg/runtime/windows/thread.c#newcode360 > ...
9 years ago (2011-09-09 11:18:04 UTC) #22
jp
On 2011/09/09 11:15:02, dvyukov wrote: > Does timer approach provides sufficient preciseness? Also it would ...
9 years ago (2011-09-09 11:36:21 UTC) #23
dvyukov
On Fri, Sep 9, 2011 at 3:36 PM, <jp@webmaster.ms> wrote: > On 2011/09/09 11:15:02, dvyukov ...
9 years ago (2011-09-09 11:50:44 UTC) #24
hector
On 2011/09/09 11:15:02, dvyukov wrote: > I do not see a problem here. The first ...
9 years ago (2011-09-09 12:02:56 UTC) #25
dvyukov
On Fri, Sep 9, 2011 at 4:02 PM, <hectorchu@gmail.com> wrote: > On 2011/09/09 11:15:02, dvyukov ...
9 years ago (2011-09-09 12:18:53 UTC) #26
dvyukov
On Fri, Sep 9, 2011 at 4:02 PM, <hectorchu@gmail.com> wrote: > > Does timer approach ...
9 years ago (2011-09-09 12:20:09 UTC) #27
dvyukov
On Fri, Sep 9, 2011 at 4:02 PM, <hectorchu@gmail.com> wrote: > >> As far as ...
9 years ago (2011-09-09 12:22:13 UTC) #28
hector
On 2011/09/09 12:18:53, dvyukov wrote: > Youre right. But then can't the following scenario happen? ...
9 years ago (2011-09-09 13:01:04 UTC) #29
dvyukov
On Fri, Sep 9, 2011 at 5:01 PM, <hectorchu@gmail.com> wrote: > On 2011/09/09 12:18:53, dvyukov ...
9 years ago (2011-09-09 13:48:51 UTC) #30
dvyukov
On Fri, Sep 9, 2011 at 5:48 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Fri, ...
9 years ago (2011-09-09 13:56:15 UTC) #31
brainman
On 2011/09/09 13:01:04, hector wrote: > > ... I will remove the prof lock from ...
9 years ago (2011-09-11 08:49:17 UTC) #32
hector
I didn't quite get the same consistent results running Dmitry's test so I started looking ...
9 years ago (2011-09-11 09:00:01 UTC) #33
hector
Ok I've determined that the most accurate way to run the timer is to use ...
9 years ago (2011-09-14 23:59:11 UTC) #34
brainman
LGTM, but I could not test it codereview issue 4960057 is out of date: patch ...
9 years ago (2011-09-15 02:45:40 UTC) #35
hector
Synced with latest, PTAL. http://codereview.appspot.com/4960057/diff/30002/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/30002/src/pkg/runtime/windows/thread.c#newcode70 src/pkg/runtime/windows/thread.c:70: runtime·stdcall(runtime·timeBeginPeriod, 1, (uintptr)1); On 2011/09/15 ...
9 years ago (2011-09-15 06:17:30 UTC) #36
brainman
On 2011/09/15 06:17:30, hector wrote: > Synced with latest, PTAL. package main import ( "fmt" ...
9 years ago (2011-09-15 06:49:52 UTC) #37
rsc
I am worried about this approach. This is using real time ticks to do the ...
9 years ago (2011-09-15 15:43:19 UTC) #38
hector
On 15 September 2011 07:49, <alex.brainman@gmail.com> wrote: > panic during panic > windows/386 Thanks. I ...
9 years ago (2011-09-15 23:16:40 UTC) #39
brainman
I think I'm stammering, but it LGTM. http://codereview.appspot.com/4960057/diff/39002/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/4960057/diff/39002/src/pkg/runtime/windows/thread.c#newcode391 src/pkg/runtime/windows/thread.c:391: thread = ...
9 years ago (2011-09-16 02:56:42 UTC) #40
hector
On Sep 16, 2011 3:56 AM, <alex.brainman@gmail.com> wrote: > > I think I'm stammering, but ...
9 years ago (2011-09-16 06:55:08 UTC) #41
rsc
LGTM
9 years ago (2011-09-16 14:20:28 UTC) #42
dvyukov
On 2011/09/16 06:55:08, hector wrote: > On Sep 16, 2011 3:56 AM, <mailto:alex.brainman@gmail.com> wrote: > ...
9 years ago (2011-09-16 18:09:59 UTC) #43
rsc
On Fri, Sep 16, 2011 at 14:09, <dvyukov@google.com> wrote: > It's really unpleasant when profiling ...
9 years ago (2011-09-16 20:38:15 UTC) #44
dvyukov
On Fri, Sep 16, 2011 at 1:38 PM, Russ Cox <rsc@golang.org> wrote: > On Fri, ...
9 years ago (2011-09-16 20:51:53 UTC) #45
rsc
On Fri, Sep 16, 2011 at 16:51, Dmitry Vyukov <dvyukov@google.com> wrote: > Yeah, but with ...
9 years ago (2011-09-16 21:10:54 UTC) #46
brainman
9 years ago (2011-09-17 07:58:10 UTC) #47
*** Submitted as http://code.google.com/p/go/source/detail?r=9c5c0cbadb4d ***

runtime: implement pprof support for windows

Credit to jp for proof of concept.

R=alex.brainman, jp, rsc, dvyukov
CC=golang-dev
http://codereview.appspot.com/4960057

Committer: Alex Brainman <alex.brainman@gmail.com>
Sign in to reply to this message.

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