The default bug view has changed. See this FAQ.

Refactor profiler management

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks: 1 bug)

unspecified
mozilla8
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 9 obsolete attachments)

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
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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
Assignee: general → sphink
(Assignee)

Comment 3

6 years ago
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.)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 547864 [details] [diff] [review]
Part 5: Remove profiling management from nsJSEnvironment.cpp
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.)
(Assignee)

Updated

6 years ago
Depends on: 671035
(Assignee)

Updated

6 years ago
Depends on: 673616
(Assignee)

Updated

6 years ago
Blocks: 675083
(Assignee)

Comment 8

6 years ago
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.
Attachment #547860 - Attachment is obsolete: true
Attachment #551194 - Flags: review?(luke)
(Assignee)

Comment 9

6 years ago
Created attachment 551195 [details] [diff] [review]
Part 2: Remove profiling management from js
Attachment #551195 - Flags: review?(luke)
(Assignee)

Comment 10

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #547861 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #547862 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
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.
Attachment #547864 - Attachment is obsolete: true
Attachment #551197 - Flags: review?(jonas)
(Assignee)

Comment 12

6 years ago
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.
Attachment #547865 - Attachment is obsolete: true
Attachment #551202 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 13

6 years ago
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.
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)
(Assignee)

Comment 16

6 years ago
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.
Attachment #547868 - Attachment is obsolete: true
Attachment #551204 - Flags: review?(luke)

Comment 17

6 years ago
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+

Updated

6 years ago
Attachment #551195 - Flags: review?(luke) → review+

Updated

6 years ago
Attachment #551203 - Flags: review?(luke) → review+

Updated

6 years ago
Attachment #551204 - Flags: review?(luke) → review+
(Assignee)

Comment 18

6 years ago
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.
Attachment #551197 - Attachment is obsolete: true
Attachment #551197 - Flags: review?(jst)
Attachment #552110 - Flags: review?(jst)

Updated

6 years ago
Attachment #552110 - Flags: review?(jst) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 552608 [details] [diff] [review]
rollup patch to land
(Assignee)

Comment 20

6 years ago
Created attachment 552610 [details] [diff] [review]
complete rollup patch
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
(Assignee)

Comment 22

6 years ago
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
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Updated

6 years ago
Depends on: 679473
(Assignee)

Updated

6 years ago
Duplicate of this bug: 672683
Depends on: 1128866
You need to log in before you can comment on or make changes to this bug.