Closed Bug 673631 Opened 8 years ago Closed 8 years ago

Refactor profiler management

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 9 obsolete files)

26.46 KB, patch
luke
: review+
Details | Diff | Splinter Review
9.10 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.79 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.32 KB, patch
luke
: review+
Details | Diff | Splinter Review
23.96 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.38 KB, patch
jst
: review+
Details | Diff | Splinter Review
63.54 KB, patch
Details | Diff | Splinter Review
64.27 KB, patch
Details | Diff | Splinter Review
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.
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.
This patch and the following ones remove the existing profiler management from where it is now
Assignee: general → sphink
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.)
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.
This patch and the previous are split out only because they are in different parts of the tree and so may need different reviewers.
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.)
Depends on: 671035
Depends on: 673616
Blocks: 675083
Convert from class to namespace, and fix some of the GC probe points.
Attachment #547860 - Attachment is obsolete: true
Attachment #551194 - Flags: review?(luke)
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.
Attachment #551195 - Attachment description: Part 2: Remove profiling management from jsprobes → Part 2: Remove profiling management from js
Attachment #547861 - Attachment is obsolete: true
Attachment #547862 - Attachment is obsolete: true
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.
Attachment #547864 - Attachment is obsolete: true
Attachment #551197 - Flags: review?(jonas)
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.
Attachment #547865 - Attachment is obsolete: true
Attachment #551202 - Flags: review?(jwalden+bmo)
This doesn't really need to be a separate patch, but it sort of cluttered up the next one.
Attachment #547863 - Attachment is obsolete: true
Attachment #551203 - Flags: review?(luke)
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.
Attachment #551202 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #551197 - Flags: review?(jonas) → review?(jst)
Advance notice: there is very good reason to r- this patch. Please suggest a better idea for error reporting than my awful UnsafeError mess.
Attachment #547868 - Attachment is obsolete: true
Attachment #551204 - Flags: review?(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
Attachment #551194 - Flags: review?(luke) → review+
Attachment #551195 - Flags: review?(luke) → review+
Attachment #551203 - Flags: review?(luke) → review+
Attachment #551204 - Flags: review?(luke) → review+
I tried building with --enable-callgrind and found a bunch more places where this stuff was lurking.
Attachment #551197 - Attachment is obsolete: true
Attachment #551197 - Flags: review?(jst)
Attachment #552110 - Flags: review?(jst)
Attachment #552110 - Flags: review?(jst) → review+
Attached patch rollup patch to land (obsolete) — Splinter Review
Attachment #552608 - Attachment is obsolete: true
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
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
Whiteboard: [inbound]
"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.
http://hg.mozilla.org/mozilla-central/rev/5b1e885539a5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 679473
Duplicate of this bug: 672683
Depends on: 1128866
You need to log in before you can comment on or make changes to this bug.