673631 - Refactor profiler management

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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:]]

Attachment: 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:]]

Attachment: 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:]]

Attachment: 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:]]

Attachment: 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:]]

Attachment: Part 5: Remove profiling management from nsJSEnvironment.cpp

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: 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:]]

Attachment: 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:]]

Attachment: 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:]]

Attachment: Part 2: Remove profiling management from js

Comment 10

[Steve Fink [:sfink] [:s:]]

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:]]

Attachment: 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:]]

Attachment: 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:]]

Attachment: 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]]

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) No longer reading bugmail consistently]

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:]]

Attachment: 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]]

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:]]

Attachment: 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:]]

Attachment: rollup patch to land

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: complete rollup patch

Comment 21

[Phil Ringnalda (:philor)]

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:]]

Attachment: 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)]

"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 (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/5b1e885539a5