687134 - Expose pccount to chrome code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Cedric Vivier [:cedricv]]

It would be great to expose the pccount stats in order to build browser/chrome coverage tools.

Comment 1

[Cedric Vivier [:cedricv]]

Attachment: Work-in-progress/Proof-of-Concept patch for Code Inspector demo.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: WIP (f8d66a792ddc)

Add hooks for starting and stopping PC count profiling, keeping track of the counts accumulated while profiling (at a runtime granularity) and getting count information for the profiled scripts.

    var utils = this.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
                  getInterface(Ci.nsIDOMWindowUtils);
    utils.startPCCountProfiling();
    utils.stopPCCountProfiling();
    var count = utils.getPCCountScriptCount();
    var json = utils.getPCCountScriptSummary(i);
    var json = utils.getPCCountScriptContents(scriptIndex);
    utils.purgePCCounts();

startPCCountProfiling discards all JIT code in the runtime and does any recompilation with PC counters on the scripts, stopPCCountProfiling also discards all code and interns all profiled scripts and associated counts in a big array which is wiped by purgePCCounts and queried by the other two methods.

getPCCountScriptSummary gets a small bit of JSON for the script (name, aggregate counts for help in sorting), getPCCountScriptContents gets detailed JSON for the opcodes in the script, pretty-printed decompilation of the script's code (in many cases web JS is minified, so pretty printing is necessary) and information to identify the precise location in the decompiled code of opcodes (needs more work).

(I'm guessing there is a better interface to hang this than nsIDOMWindowUtils, but I don't know these interfaces at all).

Comment 3

[Brian Hackett [Laid off!]]

Attachment: screen shot

Screen shot from a mockup based on CodeInspector, using these hooks to sort scripts by JIT activity and pick out hot expressions in each one.

Comment 4

[Brian Hackett [Laid off!]]

Oh, code for the mockup above will be on GitHub soon (once I figure out how to make new projects).

Comment 5

[Cedric Vivier [:cedricv]]

(In reply to Brian Hackett from comment #4)
> Oh, code for the mockup above will be on GitHub soon (once I figure out how
> to make new projects).

Nice!
Maybe you could fork CodeInspector so that we can work together on it via pull requests exchanges? :)

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Cedric Vivier [cedricv] from comment #5)
> Nice!
> Maybe you could fork CodeInspector so that we can work together on it via
> pull requests exchanges? :)

Cool, I forked CodeInspector to the link below and committed the changes I made.

https://github.com/bhackett1024/CodeInspector

Comment 7

[Brian Hackett [Laid off!]]

Attachment: WIP (920c5da54a5c)

Rework things to compute offsets in the decompiled text for all expressions, not just top level ones.  Makes addon integration a good deal simpler.  Mostly done.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: patch (920c5da54a5c)

Comment 9

[Brian Hackett [Laid off!]]

Attachment: PCCount interface changes

Splitting this patch up for review.

Steve, this part has the API hooks for starting and stopping PC counts and constructing JSON for scripts.  Everything except the decompiler changes, basically.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: compute decompiled code offsets for each op

The second part of the patch.

Jeff, this extends the decompiler to optionally compute the decompiled text for each opcode and the position in the final decompilation of those opcodes.  This is needed so that extensions can precisely tie information about the execution of a script to the actual 
decompiled code for the script (see attached screenshot).

Comment 11

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

Comment on attachment 575981 [details] [diff] [review]
PCCount interface changes

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

Just nits.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +67,5 @@
>  interface nsIDOMWindow;
>  interface nsIDOMFile;
>  interface nsIFile;
>  
> +[ptr] native JSObjectPtr (JSObject);

What's this for? I don't see it used.

::: js/src/jscompartment.cpp
@@ +550,5 @@
>              releaseTypes = false;
>  
>          /*
> +         * Don't purge types when scripts may need to be decompiled for profiling.
> +         * XXX remove when script->function() is always available.

Please file a bug and reference it here.

::: js/src/jsgc.cpp
@@ +3631,5 @@
> + * Function                 None      Profile   Query
> + * --------
> + * StartPCCountProfiling    Profile   Profile   Profile
> + * StopPCCountProfiling     None      Query     Query
> + * PurgePCCounts            None      None      None

The case of calling StartPCCountProfiling() when the state is Query was a little hard to follow. The definition of Profile is that active scripts have counter information, but it takes some investigation to discover that you're turning on counting *immediately*, at least for interpreter frames. Seems worth a comment.

Along those lines, how does that work if StartPCCountProfiling is invoked from a JITted function? It looks to me like it wouldn't start counting until the next GC? I don't see anything forcing it into the Interpoline or whatever.

At any rate, the exact semantics should be documented somewhere.

::: js/src/jsopcode.cpp
@@ +5968,5 @@
> +        if (atom) {
> +            AppendJSONProperty(buf, "name");
> +            if (!(str = JS_ValueToSource(cx, StringValue(atom))))
> +                return NULL;
> +            buf.append(str);

This string-quoting dance seems to happen often enough that you could make an AppendJSONString(buf, cx, JSString) returning bool.

@@ +6006,5 @@
> +                    propertyTotals[j - OpcodeCounts::ACCESS_COUNT] += value;
> +                    continue;
> +                }
> +                JS_NOT_REACHED("Bad opcode");
> +            } else if (OpcodeCounts::arithOp(op)) {

This 'else' is inconsistent with the rest of the structure. But I think the whole thing would read slightly better with 'else if's -- there don't seem to be any special-case fallthroughs that the continues are helping with.

@@ +6166,5 @@
> +
> +        MaybeComma comma = NO_COMMA;
> +        for (unsigned i = 0; i < numCounts; i++) {
> +            double value = counts.get(i);
> +            if (value) {

if (value > 0), maybe? I know it's fine here, but doing exact comparisons on floats gives me the heebie-jeebies. (Though I guess if you really were worried about rounding, that'd be value > 0.5. And that would just be silly here.)

::: js/src/jsopcode.h
@@ +710,5 @@
> +
> +    // Boolean conversion, for 'if (counters) ...'
> +    operator void*() const {
> +        return counts;
> +    }

I know I wrote this, but is it really a good idea? Does it make things easier or harder to follow?

::: js/src/jsopcodeinlines.h
@@ +73,5 @@
>  }
>  
> +class SrcNoteLineScanner
> +{
> +    /* offset of the current JSOp in the bytecode */

Heh. I have a patch in my queue that moves this into jsscript.h. jsopcodeinlines.h definitely seems better. But the advanceTo implementation probably ought to moved to jsopcode.cpp now.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 575984 [details] [diff] [review]
compute decompiled code offsets for each op

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

One blind man is as good as any other, I guess, for reviewing this...

For the record, I am not particularly keen on even further depending on decompilation correctness for this information.  Our decompiler code is just so clean, simple, and obviously correct, what could possibly go wrong?

::: js/src/jsopcode.cpp
@@ +1028,5 @@
> +    /*
> +     * Surrounded by parentheses when printed, which parentOffset does not
> +     * account for.
> +     */
> +    bool parentheses;

I'd prefer the name |parenthesized|.

@@ +1051,5 @@
>      Vector<JSAtom *> *localNames;   /* argument and variable names */
> +    Vector<DecompiledOpcode> *decompiledOpcodes; /* optional state for decompiled ops */
> +
> +    DecompiledOpcode &decompiled(jsbytecode *pc) {
> +        return (*decompiledOpcodes)[pc - script->code];

Add an explicit not-NULL assert here, since you're indexing into it (with a presumably small number, sure, but still).

@@ +1219,5 @@
> +        if (ntext) {
> +            memcpy((char *) ntext, text, len);
> +        } else {
> +            js_ReportOutOfMemory(ss->sprinter.context);
> +            return false;

This is better done as an |if (!ntext)| block.

@@ +1245,5 @@
> +static inline void
> +SprintOpcode(SprintStack *ss, const char *str, jsbytecode *pc,
> +             jsbytecode *parentpc, size_t startOffset)
> +{
> +    if (startOffset < 0) {

This will generate a compiler warning with some compilers.  And I don't see why they wouldn't be right to do so.  Should |startOffset| be |ptrdiff_t|?  The check as written right now is not going to have any effect.

@@ +2259,5 @@
> +                      jsbytecode **lastlvalpc, jsbytecode **lastrvalpc)
> +{
> +    const char *token;
> +    if (sn && SN_TYPE(sn) == SRC_ASSIGNOP) {
> +        if (lastop == JSOP_GETTER) {

I think both the lastop getter/setter possibilities here are dead code.  The uses here look like the assigning-getters-and-setters syntax I removed a year and a half ago:

http://whereswalden.com/2010/04/16/more-spidermonkey-changes-ancient-esoteric-very-rarely-used-syntax-for-creating-getters-and-setters-is-being-removed/

Remove it in a followup bug/patch, please.

@@ +2283,5 @@
>  InitSprintStack(JSContext *cx, SprintStack *ss, JSPrinter *jp, uintN depth)
>  {
>      INIT_SPRINTER(cx, &ss->sprinter, &cx->tempLifoAlloc(), PAREN_SLOP);
>  
> +    /* Allocate the parallel (to avoid padding) offset, opcode and bytecode stacks. */

Haters gonna hate the serial comma, yo.  (I never picked up this bad habit thanks to my parents, God and Ayn Rand.)

@@ +4014,2 @@
>                  } else {
> +                    SprintOpcode(ss, argv[0], lvalpc, pc, todo);

You might as well remove these SprintOpcodes from the if/else bodies, since they're so obviously duplicative.

@@ +4026,1 @@
>                  break;

You're leaking |argv| and |argbytecodes| here, aren't you?  Please fix this.

Comment 13

[Jason Orendorff [:jorendorff]]

Ideally we'd have detailed enough line and column number information that we wouldn't have to use the decompiler for this. Floating reconstituted code alongside the real code is not the greatest UI.

Bug 568142 is about adding column info. It has a patch, but it's old.

Comment 14

[Brian Hackett [Laid off!]]

Ideally we could do things both ways.  For many websites the real code is a minified wad of JS with the absolute minimum amount of whitespace, and the reconstituted code makes the structure of the code (if not the intent) clearer.  For this bug we also need exact column/extent information for pretty much every opcode in the script, and I fear that trying to keep track of this with source notes will eat a lot of memory.

It seems better all around if we compress and use the original source, then we can extract line/column information from that code for expressions in both the original source *and* a pretty printed version of the parse tree.

Comment 15

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de66e7bd2b98

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/de66e7bd2b98

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Get at the JSContext through IDL

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 582553 [details] [diff] [review]
Get at the JSContext through IDL

https://hg.mozilla.org/mozilla-central/rev/25f20780aa23

Comment 19

[Nickolay_Ponomarev]

dev-doc-needed: as far as I can see, the new APIs are all introduced in the "PCCount interface changes" patch <https://bugzilla.mozilla.org/attachment.cgi?id=575981&action=diff> and should be documented at https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils