Last Comment Bug 673631 - Refactor profiler management
: Refactor profiler management
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
: 672683 (view as bug list)
Depends on: 671035 673616 679473 1128866
Blocks: 675083
  Show dependency treegraph
 
Reported: 2011-07-22 17:38 PDT by Steve Fink [:sfink] [:s:]
Modified: 2015-02-03 01:13 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: convert js::Probes from a class to a namespace (24.97 KB, patch)
2011-07-22 17:42 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 2: Remove profiling management from jsprobes (7.38 KB, patch)
2011-07-22 17:43 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 3: Remove profiling management from shell (3.44 KB, patch)
2011-07-22 17:45 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 4: Rename DumpProfile functions to DumpBytecodes (3.06 KB, patch)
2011-07-22 17:48 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 5: Remove profiling management from nsJSEnvironment.cpp (2.36 KB, patch)
2011-07-22 17:49 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 6: Remove profiling management from Testharness.h (4.59 KB, patch)
2011-07-22 17:50 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 7: Add profiling management back in to jsdbgapi (23.41 KB, patch)
2011-07-22 17:57 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 1: convert js::Probes from a class to a namespace (26.46 KB, patch)
2011-08-05 17:06 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Review
Part 2: Remove profiling management from js (9.10 KB, patch)
2011-08-05 17:08 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Review
Part 3: Remove profiler control from nsJSEnvironment (2.56 KB, patch)
2011-08-05 17:14 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
Part 4: Remove profiler control from TestHarness.h (4.79 KB, patch)
2011-08-05 17:18 PDT, Steve Fink [:sfink] [:s:]
jwalden+bmo: review+
Details | Diff | Review
Part 5: Rename DumpProfile to DumpBytecodes (3.32 KB, patch)
2011-08-05 17:21 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Review
Part 6: Implement profiler control in a single place, jsdbgapi (23.96 KB, patch)
2011-08-05 17:25 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Review
Part 3: Remove profiler control from non-js/src places (5.38 KB, patch)
2011-08-10 10:03 PDT, Steve Fink [:sfink] [:s:]
jst: review+
Details | Diff | Review
rollup patch to land (42.66 KB, patch)
2011-08-11 23:43 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
complete rollup patch (63.54 KB, patch)
2011-08-11 23:52 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
complete rollup patch, with Shark fixes (64.27 KB, patch)
2011-08-15 17:22 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review

Description Steve Fink [:sfink] [:s:] 2011-07-22 17:38:05 PDT
The tree supports, in various forms and to varying degrees, several different profiler-like tools. This support is scattered across various places, and exposed as a JS interface inconsistently. Pieces exist in xpcom/tests/TestHarness.h, dom/base/nsJSEnvironment.cpp, js/src/jsdbgapi.cpp, and js/src/jsprobes.{h,cpp}.

This patch series first removes the existing support and consolidates it into jsdbgapi.cpp. As I'm typing this, it strikes me that this code might be big enough and wonky enough to be separated into a different public API header, but I'm not going to bother right now unless a reviewer requests it.

I think there are some jperf fragments floating around out there, and I haven't decided yet how to deal with those.
Comment 1 Steve Fink [:sfink] [:s:] 2011-07-22 17:42:10 PDT
Created attachment 547860 [details] [diff] [review]
Part 1: convert js::Probes from a class to a namespace

I wasn't using the class for anything, and it seems simpler to just use a namespace.

This patch also fixes up some GC probes, and moves a bunch of code out of the header file now that I'm accepting the loss of inlining for dtracing function calls.

This patch depends on bug 671035.
Comment 2 Steve Fink [:sfink] [:s:] 2011-07-22 17:43:22 PDT
Created attachment 547861 [details] [diff] [review]
Part 2: Remove profiling management from jsprobes

This patch and the following ones remove the existing profiler management from where it is now
Comment 3 Steve Fink [:sfink] [:s:] 2011-07-22 17:45:22 PDT
Created attachment 547862 [details] [diff] [review]
Part 3: Remove profiling management from shell

Remove profiling management function registration and help messages from the shell. (The help messages will return in a later patch; the function registration will move to jsdbgapi.cpp.)
Comment 4 Steve Fink [:sfink] [:s:] 2011-07-22 17:48:11 PDT
Created attachment 547863 [details] [diff] [review]
Part 4: Rename DumpProfile functions to DumpBytecodes

I'll be reusing the term "dump a profile" to refer to Callgrind-style profile dumping, so I need to rename the existing functions that sound like they do the same thing. In fact, all they do is dump out the bytecode disassembly of scripts, which implicitly includes the counters that the "profiling" was referring to.
Comment 5 Steve Fink [:sfink] [:s:] 2011-07-22 17:49:12 PDT
Created attachment 547864 [details] [diff] [review]
Part 5: Remove profiling management from nsJSEnvironment.cpp
Comment 6 Steve Fink [:sfink] [:s:] 2011-07-22 17:50:06 PDT
Created attachment 547865 [details] [diff] [review]
Part 6: Remove profiling management from Testharness.h

This patch and the previous are split out only because they are in different parts of the tree and so may need different reviewers.
Comment 7 Steve Fink [:sfink] [:s:] 2011-07-22 17:57:32 PDT
Created attachment 547868 [details] [diff] [review]
Part 7: Add profiling management back in to jsdbgapi

Finally, add all of the profiling management back into jsdbgapi.{h,cpp}. This includes function registration, which is a behavior change: now nsJSEnvironment-created global objects have the same profiling-related functions as does the JS shell. The full set is larger than either original subset.

There's a stunningly ugly portion of this patch: because profiling is controlled from both the JS shell (which has a JSContext for error reporting) and from TestHarness.h (which would report errors some other way -- NSPR stuff, maybe?), I can't just pass in a JSContext* and use it to report errors. So I made an errno-style global variable that callers can query on failure and handle errors appropriately. I'd love to hear ideas for better options. (I could pass in an error reporting callback, perhaps, or set it via JS_SetProfilingErrorHandler.) These errors really aren't that critical, so perhaps I should be dumping them to stderr, only I don't know if I can even do that portably. They're interesting enough that I don't want to just swallow them.

Also note that the Vtune support is for an older API, and there's still no way to start/stop ETW reporting (which is partly what set me off on this whole cleanup effort in the first place.)
Comment 8 Steve Fink [:sfink] [:s:] 2011-08-05 17:06:49 PDT
Created attachment 551194 [details] [diff] [review]
Part 1: convert js::Probes from a class to a namespace

Convert from class to namespace, and fix some of the GC probe points.
Comment 9 Steve Fink [:sfink] [:s:] 2011-08-05 17:08:17 PDT
Created attachment 551195 [details] [diff] [review]
Part 2: Remove profiling management from js
Comment 10 Steve Fink [:sfink] [:s:] 2011-08-05 17:09:07 PDT
Comment on attachment 551195 [details] [diff] [review]
Part 2: Remove profiling management from js

Ack. Hit enter too early.

This combines the previous parts 2 & 3.
Comment 11 Steve Fink [:sfink] [:s:] 2011-08-05 17:14:52 PDT
Created attachment 551197 [details] [diff] [review]
Part 3: Remove profiler control from nsJSEnvironment

I am moving profiler control out of the various places that do it into jsdbgapi (so that everyone will get the same set of profiler-related functions.) This review is implicitly for the move, not just the removal.
Comment 12 Steve Fink [:sfink] [:s:] 2011-08-05 17:18:27 PDT
Created attachment 551202 [details] [diff] [review]
Part 4: Remove profiler control from TestHarness.h

Remove profiler control from TestHarness.h because I am consolidating it in jsdbgapi. The review is for the move, not the removal.

Waldo, I pegged you because this file hasn't been changed since CVS days and your name is in the header.
Comment 13 Steve Fink [:sfink] [:s:] 2011-08-05 17:21:11 PDT
Created attachment 551203 [details] [diff] [review]
Part 5: Rename DumpProfile to DumpBytecodes

This doesn't really need to be a separate patch, but it sort of cluttered up the next one.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-05 17:24:18 PDT
Comment on attachment 551202 [details] [diff] [review]
Part 4: Remove profiler control from TestHarness.h

Review of attachment 551202 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me.  A smaller TestHarness.h is a better one, as far as making it easy for people to see what it does and can do so they can use it.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-05 17:25:14 PDT
Comment on attachment 551197 [details] [diff] [review]
Part 3: Remove profiler control from nsJSEnvironment

I'm not the right person to review this. Jst and mrbkap knows contexts better.
Comment 16 Steve Fink [:sfink] [:s:] 2011-08-05 17:25:51 PDT
Created attachment 551204 [details] [diff] [review]
Part 6: Implement profiler control in a single place, jsdbgapi

Advance notice: there is very good reason to r- this patch. Please suggest a better idea for error reporting than my awful UnsafeError mess.
Comment 17 Luke Wagner [:luke] 2011-08-08 10:55:43 PDT
Comment on attachment 551194 [details] [diff] [review]
Part 1: convert js::Probes from a class to a namespace

Review of attachment 551194 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsprobes.cpp
@@ +122,5 @@
> + * tracked, not the state of each profiler.
> + *
> + * Return the previous state.
> + */
> +static bool pauseProfilers(JSContext *cx) {

Curly on the next line, here and below.

::: js/src/jsprobes.h
@@ +355,5 @@
>      return ok;
>  }
>  
> +#ifdef INCLUDE_MOZILLA_DTRACE
> +static const char *ObjectClassname(JSObject *obj) {

curly
Comment 18 Steve Fink [:sfink] [:s:] 2011-08-10 10:03:18 PDT
Created attachment 552110 [details] [diff] [review]
Part 3: Remove profiler control from non-js/src places

I tried building with --enable-callgrind and found a bunch more places where this stuff was lurking.
Comment 19 Steve Fink [:sfink] [:s:] 2011-08-11 23:43:31 PDT
Created attachment 552608 [details] [diff] [review]
rollup patch to land
Comment 20 Steve Fink [:sfink] [:s:] 2011-08-11 23:52:34 PDT
Created attachment 552610 [details] [diff] [review]
complete rollup patch
Comment 21 Phil Ringnalda (:philor) 2011-08-12 19:28:01 PDT
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/d7ccb99a2f2d since it made the Shark shell build squeal

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1313187998.1313188121.27766.gz
jsdbgapi.cpp
g++-4.2 -o jsdbgapi.o -c  -fvisibility=hidden -DOSTYPE=\"Darwin9.2.0\" -DOSARCH=Darwin -DEXPORT_JS_API -DIMPL_MFBT  -I../../src/js/src -I. -I/builds/slave/m-in-osx-dbg-spidermonkey-shark/objdir/dist/include -I/builds/slave/m-in-osx-dbg-spidermonkey-shark/objdir/dist/include/nsprpub  -I/builds/slave/m-in-osx-dbg-spidermonkey-shark/objdir/dist/include/nspr   -I../../src/js/src -I../../src/js/src/assembler -I../../src/js/src/yarr  -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -fno-strict-aliasing -fpascal-strings -fno-common -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g -O3 -fstrict-aliasing -fno-stack-protector -fno-omit-frame-pointer -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1   -DMOZILLA_CLIENT -include ./js-confdefs.h -MD -MF .deps/jsdbgapi.pp /builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp: In function 'JSBool JS_StartProfiling(const char*)':
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp:1542: error: 'Shark' has not been declared
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp: In function 'JSBool JS_StopProfiling(const char*)':
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp:1559: error: 'Shark' has not been declared
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp: In function 'JSBool ControlProfilers(bool)':
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp:1579: error: 'Shark' has not been declared
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp:1580: error: 'profileName' was not declared in this scope
/builds/slave/m-in-osx-dbg-spidermonkey-shark/src/js/src/jsdbgapi.cpp:1596: error: 'Shark' has not been declared
Comment 22 Steve Fink [:sfink] [:s:] 2011-08-15 17:22:59 PDT
Created attachment 553315 [details] [diff] [review]
complete rollup patch, with Shark fixes

Ok, compiled it on OSX and ran Shark against it successfully, using startProfiling() and stopProfiling() to start/stop.

http://hg.mozilla.org/integration/mozilla-inbound/rev/5b1e885539a5
Comment 23 Phil Ringnalda (:philor) 2011-08-15 23:33:57 PDT
"jsdbgapi.cpp:1233: warning: 'void UnsafeError(const char*, ...)' defined but not used" but I'm pretty numb by now, so I just hid the 10.5 opt and debug warnaserror builds.
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-16 04:07:28 PDT
http://hg.mozilla.org/mozilla-central/rev/5b1e885539a5
Comment 25 Steve Fink [:sfink] [:s:] 2011-09-09 22:12:07 PDT
*** Bug 672683 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.