Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10454076:
Signal thread names in instrumented binaries, even when a debugger is not attached. (Closed)
|
Created:
8 years, 6 months ago by Sigurður Ásgeirsson Modified:
8 years, 6 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSignal thread names in instrumented binaries, even when a debugger is not attached.
Cache the determiniation whether a binary is instrumented or not, as it'll never change for a given binary.
R=jar@google.com
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139777
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address Jim's comments. #
Total comments: 1
Messages
Total messages: 11 (0 generated)
Hey Jim, with this change to base, SyzyProf is able to break down profile data by human-readable thread name. PTAL. Siggi
Which binaries are you planning on instrumenting? Are all release binaries going to be instrumented? ... and then how much does this annotation cost? If it is really really cheap, why don't we just do it all the time? Any idea what the range of times are? Note that I've heard we have about 50 threads these days... but I'm not sure... and not sure how many are named. https://chromiumcodereview.appspot.com/10454076/diff/1/base/debug/profiler.cc File base/debug/profiler.cc (right): https://chromiumcodereview.appspot.com/10454076/diff/1/base/debug/profiler.cc... base/debug/profiler.cc:103: // This should be self-evident, soon as we're executing. nit: This was a strange comment. Are you saying in the comment that: "This comment is not necessary." If the comment is true, it should not be here ;-). I wasn't familiar with VerifyMagic(), so IF you wanted to comment, it might be something more like: // Check to be sure or image is structured as we'd expect. https://chromiumcodereview.appspot.com/10454076/diff/1/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://chromiumcodereview.appspot.com/10454076/diff/1/base/threading/platfor... base/threading/platform_thread_win.cc:139: if (!::IsDebuggerPresent() && !base::debug::IsBinaryInstrumented()) FWIW: In the about:profiler world, we depend on a Thread object to provide its name, rather than the OS (which is really what is getting set here). Have you considered relying on that instead of the OS setting, which the debugger relies on (an we can't change the debugger)?
On Wed, May 30, 2012 at 3:41 PM, <jar@chromium.org> wrote: > Which binaries are you planning on instrumenting? Are all release binaries > going to be instrumented? ... and then how much does this annotation > cost? If > it is really really cheap, why don't we just do it all the time? Any idea > what > the range of times are? > > Instrumentation will be done at need, prior to profiling, by the developer wanting to profile a given binary. It's the goal of SyzyProf to make it (eventually) possible to to instrument any Chrome or Chromium binary you build locally, or that you encounter in the wild. We will not be shipping any instrumented binaries to users, however. > Note that I've heard we have about 50 threads these days... but I'm not > sure... > and not sure how many are named. > > The thread constructor seems to take a name, and in my testing it looks like all threads except the initial (main) thread are named. > > https://chromiumcodereview.**appspot.com/10454076/diff/1/** > base/debug/profiler.cc<https://chromiumcodereview.appspot.com/10454076/diff/1/base/debug/profiler.cc> > File base/debug/profiler.cc (right): > > https://chromiumcodereview.**appspot.com/10454076/diff/1/** > base/debug/profiler.cc#**newcode103<https://chromiumcodereview.appspot.com/10454076/diff/1/base/debug/profiler.cc#newcode103> > base/debug/profiler.cc:103: // This should be self-evident, soon as > we're executing. > nit: This was a strange comment. > Are you saying in the comment that: "This comment is not necessary." > > If the comment is true, it should not be here ;-). > > I wasn't familiar with VerifyMagic(), so IF you wanted to comment, it > might be something more like: > // Check to be sure or image is structured as we'd expect. > > https://chromiumcodereview.**appspot.com/10454076/diff/1/** > base/threading/platform_**thread_win.cc<https://chromiumcodereview.appspot.com/10454076/diff/1/base/threading/platform_thread_win.cc> > File base/threading/platform_**thread_win.cc (right): > > https://chromiumcodereview.**appspot.com/10454076/diff/1/** > base/threading/platform_**thread_win.cc#newcode139<https://chromiumcodereview.appspot.com/10454076/diff/1/base/threading/platform_thread_win.cc#newcode139> > base/threading/platform_**thread_win.cc:139: if (!::IsDebuggerPresent() && > !base::debug::**IsBinaryInstrumented()) > FWIW: In the about:profiler world, we depend on a Thread object to > provide its name, rather than the OS (which is really what is getting > set here). > > Have you considered relying on that instead of the OS setting, which the > debugger relies on (an we can't change the debugger)? > > The profiler is loosely coupled from Chrome binaries, as in it's a separate DLL, and this is an existing thread naming mechanism that's used by all manner of Windows libraries. I could make Chrome declare thread names by looking up an exported function on the profile instrumentation DLL, but that seems like a lot of extra hassle for a net loss. This way there's an extra flag check for each new thread created. By communicating through an export it'll look very similar, except it'll check whether a function pointer is NULL and invoke on it if not. > https://chromiumcodereview.**appspot.com/10454076/<https://chromiumcodereview... >
Thanks! Please take another look. https://chromiumcodereview.appspot.com/10454076/diff/1/base/debug/profiler.cc File base/debug/profiler.cc (right): https://chromiumcodereview.appspot.com/10454076/diff/1/base/debug/profiler.cc... base/debug/profiler.cc:103: // This should be self-evident, soon as we're executing. On 2012/05/30 19:41:23, jar wrote: > nit: This was a strange comment. > Are you saying in the comment that: "This comment is not necessary." > > If the comment is true, it should not be here ;-). > > I wasn't familiar with VerifyMagic(), so IF you wanted to comment, it might be > something more like: > // Check to be sure or image is structured as we'd expect. Done. https://chromiumcodereview.appspot.com/10454076/diff/1/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://chromiumcodereview.appspot.com/10454076/diff/1/base/threading/platfor... base/threading/platform_thread_win.cc:139: if (!::IsDebuggerPresent() && !base::debug::IsBinaryInstrumented()) On 2012/05/30 19:41:23, jar wrote: > FWIW: In the about:profiler world, we depend on a Thread object to provide its > name, rather than the OS (which is really what is getting set here). > > Have you considered relying on that instead of the OS setting, which the > debugger relies on (an we can't change the debugger)? Yups, as per my previous message, this is more useful as it uses an existing "interface" or convention also used by other windows libraries. It also allows the profiler and chrome binaries to be loosely coupled.
Sorry... on more question... https://chromiumcodereview.appspot.com/10454076/diff/5001/base/debug/profiler.cc File base/debug/profiler.cc (right): https://chromiumcodereview.appspot.com/10454076/diff/5001/base/debug/profiler... base/debug/profiler.cc:90: bool IsBinaryInstrumented() { Does pulling in this function actually pull in all code in this file... and does that pull in stuff from other files? Is this really a minimal growth of the executable foot print? Is this really a tiny stub, and the real profiler bulk is linked in dynamically on demand? [I assume you watch this class of stuff... and I assume that most of the profiler is not currently in Chrome... but perhaps I'll be surprised! ;-) ]
On Wed, May 30, 2012 at 6:30 PM, <jar@chromium.org> wrote: > Sorry... on more question... > > > https://chromiumcodereview.**appspot.com/10454076/diff/** > 5001/base/debug/profiler.cc<https://chromiumcodereview.appspot.com/10454076/diff/5001/base/debug/profiler.cc> > File base/debug/profiler.cc (right): > > https://chromiumcodereview.**appspot.com/10454076/diff/** > 5001/base/debug/profiler.cc#**newcode90<https://chromiumcodereview.appspot.com/10454076/diff/5001/base/debug/profiler.cc#newcode90> > base/debug/profiler.cc:90: bool IsBinaryInstrumented() { > Does pulling in this function actually pull in all code in this file... > and does that pull in stuff from other files? > > Is this really a minimal growth of the executable foot print? Is this > really a tiny stub, and the real profiler bulk is linked in dynamically > on demand? > > Yups, that's how it works. When the instrumenter instruments the binary, it injects an extra DLL dependency to the instrumented copy of the binary, and adds a couple of sections to it. This code merely tests for the presence of those new sections, that will only be present in instrumented copies of the original, as a signal that the binary is indeed instrumented. We also use this same probe as a trigger to set up a profiling hook in V8, and now that it's called once per thread instead of once per process, I felt it would be better to cache the results of the probing. > [I assume you watch this class of stuff... and I assume that most of the > profiler is not currently in Chrome... but perhaps I'll be surprised! > ;-) ] > > https://chromiumcodereview.**appspot.com/10454076/<https://chromiumcodereview... >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/10454076/5001
Try job failure for 10454076-5001 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siggi@chromium.org/10454076/5001
Change committed as 139777 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
