Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 687134 - Expose pccount to chrome code
: Expose pccount to chrome code
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla11
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on: 716702
Blocks: 771118
  Show dependency treegraph
Reported: 2011-09-16 12:14 PDT by Cedric Vivier [:cedricv]
Modified: 2012-07-05 06:48 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Work-in-progress/Proof-of-Concept patch for Code Inspector demo. (7.45 KB, patch)
2011-09-16 12:15 PDT, Cedric Vivier [:cedricv]
no flags Details | Diff | Splinter Review
WIP (f8d66a792ddc) (45.72 KB, patch)
2011-11-07 22:48 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
screen shot (212.62 KB, image/png)
2011-11-07 22:54 PST, Brian Hackett (:bhackett)
no flags Details
WIP (920c5da54a5c) (88.91 KB, patch)
2011-11-14 16:51 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
patch (920c5da54a5c) (107.45 KB, patch)
2011-11-17 15:47 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
PCCount interface changes (42.05 KB, patch)
2011-11-21 14:46 PST, Brian Hackett (:bhackett)
sphink: review+
Details | Diff | Splinter Review
compute decompiled code offsets for each op (65.46 KB, patch)
2011-11-21 14:52 PST, Brian Hackett (:bhackett)
jwalden+bmo: review+
Details | Diff | Splinter Review
Get at the JSContext through IDL (4.25 KB, patch)
2011-12-17 11:11 PST, :Ms2ger (⌚ UTC+1/+2)
bobbyholley: review+
Ms2ger: checkin+
Details | Diff | Splinter Review

Description Cedric Vivier [:cedricv] 2011-09-16 12:14:17 PDT
It would be great to expose the pccount stats in order to build browser/chrome coverage tools.
Comment 1 Cedric Vivier [:cedricv] 2011-09-16 12:15:02 PDT
Created attachment 560603 [details] [diff] [review]
Work-in-progress/Proof-of-Concept patch for Code Inspector demo.
Comment 2 Brian Hackett (:bhackett) 2011-11-07 22:48:09 PST
Created attachment 572744 [details] [diff] [review]
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).
    var count = utils.getPCCountScriptCount();
    var json = utils.getPCCountScriptSummary(i);
    var json = utils.getPCCountScriptContents(scriptIndex);

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 (:bhackett) 2011-11-07 22:54:44 PST
Created attachment 572746 [details]
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 (:bhackett) 2011-11-07 22:56:05 PST
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] 2011-11-07 23:09:26 PST
(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).

Maybe you could fork CodeInspector so that we can work together on it via pull requests exchanges? :)
Comment 6 Brian Hackett (:bhackett) 2011-11-08 10:25:44 PST
(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.
Comment 7 Brian Hackett (:bhackett) 2011-11-14 16:51:10 PST
Created attachment 574478 [details] [diff] [review]
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 (:bhackett) 2011-11-17 15:47:12 PST
Created attachment 575316 [details] [diff] [review]
patch (920c5da54a5c)
Comment 9 Brian Hackett (:bhackett) 2011-11-21 14:46:55 PST
Created attachment 575981 [details] [diff] [review]
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 (:bhackett) 2011-11-21 14:52:36 PST
Created attachment 575984 [details] [diff] [review]
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:] 2011-11-23 16:26:33 PST
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] (remove +bmo to email) 2011-12-08 09:48:26 PST
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:

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] 2011-12-12 17:15:56 PST
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 (:bhackett) 2011-12-12 17:57:34 PST
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 (:bhackett) 2011-12-16 13:12:33 PST
Comment 16 Matt Brubeck (:mbrubeck) 2011-12-17 09:23:13 PST
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2011-12-17 11:11:18 PST
Created attachment 582553 [details] [diff] [review]
Get at the JSContext through IDL
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-12-24 04:31:17 PST
Comment on attachment 582553 [details] [diff] [review]
Get at the JSContext through IDL
Comment 19 Nickolay_Ponomarev 2012-01-01 14:28:24 PST
dev-doc-needed: as far as I can see, the new APIs are all introduced in the "PCCount interface changes" patch <> and should be documented at

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