Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Feb 25 00:47, Mark Geisert wrote:
> On Tue, 23 Feb 2016, Corinna Vinschen wrote:
> >On Feb 22 23:36, Mark Geisert wrote:
> >>On Mon, 22 Feb 2016, Jon Turney wrote:
> >>>There doesn't seem to be anything specific to profiling about this, so it
> >>>could be written in a more generic way, as "call a callback function for
> >>>each thread".
> >>
> >>I saw your later conversation with Corinna on the list re why
> >>cygwin_internal() is involved now. (I too had stumbled over the
> >>cygwin1.dll/libgmon.a gap when I started this work.) Given the necessity of
> >>the separation, does it still make sense to write a generic per-thread
> >>callback mechanism and then make use of it for this patch, or is that
> >>overkill? I can't tell.
> >
> >One problem with a generic solution is to generalize the arguments to
> >the called function. IMHO, keep it as is for now. If we ever need to
> >make this generic we can still do it.
>
> OK.
>
> + if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> >>>
> >>>setup-env.xml might be an appropriate place to mention this environment
> >>>variable.
> >>
> >>I am now writing a gprof.xml that will be tied into the existing
> >>programming.xml. I plan to document GMON_OUT_PREFIX in gprof.xml. Do you
> >>think that's sufficient?
> >
> >A single paragraph in setup-env.xml pointing to gprof.xml might be
> >helpful, I think.
>
> Alright, I can do that as part of the separate doc patch that I'm working.
>
> I ran into an issue while testing the profiling of programs that fork() but
> don't exec(). So it may be a short while before I can send ver 3. Just FYI.
Ok, no worries. If you want to discuss something, we also have the
#cygwin-developers IRC channel on freenode.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Tue, 23 Feb 2016, Corinna Vinschen wrote:
On Feb 22 23:36, Mark Geisert wrote:
On Mon, 22 Feb 2016, Jon Turney wrote:
There doesn't seem to be anything specific to profiling about this, so it
could be written in a more generic way, as "call a callback function for
each thread".
I saw your later conversation with Corinna on the list re why
cygwin_internal() is involved now. (I too had stumbled over the
cygwin1.dll/libgmon.a gap when I started this work.) Given the necessity of
the separation, does it still make sense to write a generic per-thread
callback mechanism and then make use of it for this patch, or is that
overkill? I can't tell.
One problem with a generic solution is to generalize the arguments to
the called function. IMHO, keep it as is for now. If we ever need to
make this generic we can still do it.
OK.
+ if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
setup-env.xml might be an appropriate place to mention this environment
variable.
I am now writing a gprof.xml that will be tied into the existing
programming.xml. I plan to document GMON_OUT_PREFIX in gprof.xml. Do you
think that's sufficient?
A single paragraph in setup-env.xml pointing to gprof.xml might be
helpful, I think.
Alright, I can do that as part of the separate doc patch that I'm working.
I ran into an issue while testing the profiling of programs that fork()
but don't exec(). So it may be a short while before I can send ver 3.
Just FYI.
..mark
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Feb 22 23:36, Mark Geisert wrote:
> Hi Jon,
>
> On Mon, 22 Feb 2016, Jon Turney wrote:
> >Thanks for this. A few comments inline.
> >
> >On 20/02/2016 08:16, Mark Geisert wrote:
> >>+/* Called from profil.c to sample all non-main thread PC values for
> >>profiling */
> >>+extern "C" void
> >>+cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
> >>+{
> >>+ for (uint32_t ix = 0; ix < nthreads; ix++)
> >>+{
> >>+ _cygtls *tls = cygheap->threadlist[ix].thread;
> >>+ if (tls->tid)
> >>+ profthr_byhandle (tls->tid->win32_obj_id);
> >>+}
> >>+}
> >
> >There doesn't seem to be anything specific to profiling about this, so it
> >could be written in a more generic way, as "call a callback function for
> >each thread".
>
> I saw your later conversation with Corinna on the list re why
> cygwin_internal() is involved now. (I too had stumbled over the
> cygwin1.dll/libgmon.a gap when I started this work.) Given the necessity of
> the separation, does it still make sense to write a generic per-thread
> callback mechanism and then make use of it for this patch, or is that
> overkill? I can't tell.
One problem with a generic solution is to generalize the arguments to
the called function. IMHO, keep it as is for now. If we ever need to
make this generic we can still do it.
> >>+ if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> >
> >setup-env.xml might be an appropriate place to mention this environment
> >variable.
>
> I am now writing a gprof.xml that will be tied into the existing
> programming.xml. I plan to document GMON_OUT_PREFIX in gprof.xml. Do you
> think that's sufficient?
A single paragraph in setup-env.xml pointing to gprof.xml might be
helpful, I think.
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Feb 22 22:58, Mark Geisert wrote: > On Mon, 22 Feb 2016, Corinna Vinschen wrote: > >One is, for completeness it would be nice if you could add a > >description to the git comment along the lines of your original > >comment so we have a description in the log. > > Sorry, can't parse this; git newbie here. Did you mean the 'git commit' I'm > doing to my private repository and the message associated with the commit? Yes, exactly. > And by "original comment" do you mean what I called the change log in the > text of my v2 email we're discussing (i.e., not the patch attachment but the > email body)? No, I mean the first patch submission. Your v1 patch submission had a nice explaining text. It might be helpful to have this text (tweaked to the v2 changes) in the git log, together with the ChangeLog. > >The other point is: > >>+ long divisor = 10; // the power of 10 bigger than PID_MAX > > > >I've seen 6 digit PIDs. In fact, we're not that tight on space here > >so we should err on the side of caution and leave room for the entire > >possible size of a Windows PID. That's a LONG, 32 bit, 10 decimal > >digits. > > Yikes. I'd seen large 5-digit pids but could not find a definitive symbol > defining Windows' maximum pid value. So I will change divisor's init value > to 1000*1000*1000 which will allow the conversion loop to support 10-digit > pids. ACK. > >Other than that, the patch looks good to me. > > Great! I'll follow up with Jon separately (to the list) on his comments. Yup. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
Hi Jon,
On Mon, 22 Feb 2016, Jon Turney wrote:
Thanks for this. A few comments inline.
On 20/02/2016 08:16, Mark Geisert wrote:
+/* Called from profil.c to sample all non-main thread PC values for
profiling */
+extern "C" void
+cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
+{
+ for (uint32_t ix = 0; ix < nthreads; ix++)
+{
+ _cygtls *tls = cygheap->threadlist[ix].thread;
+ if (tls->tid)
+ profthr_byhandle (tls->tid->win32_obj_id);
+}
+}
There doesn't seem to be anything specific to profiling about this, so it
could be written in a more generic way, as "call a callback function for each
thread".
I saw your later conversation with Corinna on the list re why
cygwin_internal() is involved now. (I too had stumbled over the
cygwin1.dll/libgmon.a gap when I started this work.) Given the necessity
of the separation, does it still make sense to write a generic per-thread
callback mechanism and then make use of it for this patch, or is that
overkill? I can't tell.
+ if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
setup-env.xml might be an appropriate place to mention this environment
variable.
I am now writing a gprof.xml that will be tied into the existing
programming.xml. I plan to document GMON_OUT_PREFIX in gprof.xml. Do you
think that's sufficient?
..mark
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Mon, 22 Feb 2016, Corinna Vinschen wrote: One is, for completeness it would be nice if you could add a description to the git comment along the lines of your original comment so we have a description in the log. Sorry, can't parse this; git newbie here. Did you mean the 'git commit' I'm doing to my private repository and the message associated with the commit? And by "original comment" do you mean what I called the change log in the text of my v2 email we're discussing (i.e., not the patch attachment but the email body)? The other point is: + long divisor = 10; // the power of 10 bigger than PID_MAX I've seen 6 digit PIDs. In fact, we're not that tight on space here so we should err on the side of caution and leave room for the entire possible size of a Windows PID. That's a LONG, 32 bit, 10 decimal digits. Yikes. I'd seen large 5-digit pids but could not find a definitive symbol defining Windows' maximum pid value. So I will change divisor's init value to 1000*1000*1000 which will allow the conversion loop to support 10-digit pids. Other than that, the patch looks good to me. Great! I'll follow up with Jon separately (to the list) on his comments. ..mark
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On 22/02/2016 16:55, Corinna Vinschen wrote: On Feb 22 16:14, Jon Turney wrote: On 22/02/2016 14:25, Corinna Vinschen wrote: On Feb 22 11:44, Jon Turney wrote: Thanks for this. A few comments inline. [...] On 20/02/2016 08:16, Mark Geisert wrote: + // record profiling samples for other pthreads, if any + cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle); + Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ? I asked for it: https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html Ok. I guess I don't understand why it's exported at all in that version of the patch. I don't understand the question. cygheap_profthr_all isn't exported anymore in v2. I wasn't taking into account the fact that profil.c gets built into a separate library, libgmon.a. So that clears up my misunderstanding as to why this operation needs to be exposed outside the DLL at all.
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Feb 22 16:14, Jon Turney wrote: > On 22/02/2016 14:25, Corinna Vinschen wrote: > >On Feb 22 11:44, Jon Turney wrote: > >>Thanks for this. A few comments inline. > >>[...] > >>On 20/02/2016 08:16, Mark Geisert wrote: > >>>+ // record profiling samples for other pthreads, if any > >>>+ cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle); > >>>+ > >> > >>Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ? > > > >I asked for it: > >https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html > > Ok. I guess I don't understand why it's exported at all in that version of > the patch. I don't understand the question. cygheap_profthr_all isn't exported anymore in v2. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On 22/02/2016 14:25, Corinna Vinschen wrote: On Feb 22 11:44, Jon Turney wrote: Thanks for this. A few comments inline. [...] On 20/02/2016 08:16, Mark Geisert wrote: + // record profiling samples for other pthreads, if any + cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle); + Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ? I asked for it: https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html Ok. I guess I don't understand why it's exported at all in that version of the patch.
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
On Feb 22 11:44, Jon Turney wrote: > Thanks for this. A few comments inline. > [...] > On 20/02/2016 08:16, Mark Geisert wrote: > >+ // record profiling samples for other pthreads, if any > >+ cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle); > >+ > > Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ? I asked for it: https://cygwin.com/ml/cygwin-patches/2016-q1/msg00028.html Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
Thanks for this. A few comments inline.
On 20/02/2016 08:16, Mark Geisert wrote:
diff --git a/winsup/cygwin/cygheap.cc b/winsup/cygwin/cygheap.cc
index 6493485..4932cf0 100644
--- a/winsup/cygwin/cygheap.cc
+++ b/winsup/cygwin/cygheap.cc
@@ -744,3 +744,15 @@ init_cygheap::find_tls (int sig, bool& issig_wait)
WaitForSingleObject (t->mutex, INFINITE);
return t;
}
+
+/* Called from profil.c to sample all non-main thread PC values for profiling
*/
+extern "C" void
+cygheap_profthr_all (void (*profthr_byhandle) (HANDLE))
+{
+ for (uint32_t ix = 0; ix < nthreads; ix++)
+{
+ _cygtls *tls = cygheap->threadlist[ix].thread;
+ if (tls->tid)
+ profthr_byhandle (tls->tid->win32_obj_id);
+}
+}
There doesn't seem to be anything specific to profiling about this, so
it could be written in a more generic way, as "call a callback function
for each thread".
diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
index e379df1..02335eb 100644
--- a/winsup/cygwin/external.cc
+++ b/winsup/cygwin/external.cc
@@ -702,6 +702,17 @@ cygwin_internal (cygwin_getinfo_types t, ...)
}
break;
+ case CW_CYGHEAP_PROFTHR_ALL:
+ {
+ typedef void (*func_t) (HANDLE);
+ extern void cygheap_profthr_all (func_t);
+
+ func_t profthr_byhandle = va_arg(arg, func_t);
+ cygheap_profthr_all (profthr_byhandle);
+ res = 0;
+ }
+ break;
+
Is this exposed via cygwin_internal() operation just for testing
purposes? Or is there some other use envisioned?
+ /* We copy an undocumented glibc feature: customizing the profiler's
+ output file name somewhat, depending on the env var GMON_OUT_PREFIX.
+ if GMON_OUT_PREFIX is unspecified, the file's name is "gmon.out".
+
+ if GMON_OUT_PREFIX is specified with at least one character, the
+ file's name is computed as "$GMON_OUT_PREFIX.$pid".
+
+ if GMON_OUT_PREFIX is specified but contains no characters, the
+ file's name is computed as "gmon.out.$pid". Cygwin-specific.
+ */
+ if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
setup-env.xml might be an appropriate place to mention this environment
variable.
static void CALLBACK
profthr_func (LPVOID arg)
{
struct profinfo *p = (struct profinfo *) arg;
- size_t pc, idx;
for (;;)
{
- pc = (size_t) get_thrpc (p->targthr);
- if (pc >= p->lowpc && pc < p->highpc)
- {
- idx = PROFIDX (pc, p->lowpc, p->scale);
- p->counter[idx]++;
- }
+ // record profiling sample for main thread
+ profthr_byhandle (p->targthr);
+
+ // record profiling samples for other pthreads, if any
+ cygwin_internal (CW_CYGHEAP_PROFTHR_ALL, profthr_byhandle);
+
Hmm.. so why isn't this written as cygheap_profthr_all (profthr_byhandle) ?
Re: [PATCH] gprof profiling of multi-threaded Cygwin programs, ver 2
Hi Mark,
On Feb 20 00:16, Mark Geisert wrote:
> Version 2 incorporating review comments of version 1.
I'm afraid we need a v3, two points.
One is, for completeness it would be nice if you could add a
description to the git comment along the lines of your original
comment so we have a description in the log.
The other point is:
> + /* We copy an undocumented glibc feature: customizing the profiler's
> +output file name somewhat, depending on the env var GMON_OUT_PREFIX.
> +if GMON_OUT_PREFIX is unspecified, the file's name is "gmon.out".
> +
> +if GMON_OUT_PREFIX is specified with at least one character, the
> +file's name is computed as "$GMON_OUT_PREFIX.$pid".
> +
> +if GMON_OUT_PREFIX is specified but contains no characters, the
> +file's name is computed as "gmon.out.$pid". Cygwin-specific.
> + */
> + if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> + char *buf;
> + long divisor = 10; // the power of 10 bigger than PID_MAX
> + pid_t pid = getpid();
> + size_t len = strlen(prefix);
> +
> + if (len == 0)
> + len = strlen(prefix = filename);
> + buf = alloca(len + 8); // allows for '.', 5-digit pid, NUL, +1
I've seen 6 digit PIDs. In fact, we're not that tight on space here
so we should err on the side of caution and leave room for the entire
possible size of a Windows PID. That's a LONG, 32 bit, 10 decimal
digits.
Other than that, the patch looks good to me.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
