Expose pccount to chrome code

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cedricv, Assigned: bhackett)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
It would be great to expose the pccount stats in order to build browser/chrome coverage tools.
(Reporter)

Comment 1

6 years ago
Created attachment 560603 [details] [diff] [review]
Work-in-progress/Proof-of-Concept patch for Code Inspector demo.
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

6 years ago
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.
Assignee: general → bhackett1024
Attachment #572744 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 575316 [details] [diff] [review]
patch (920c5da54a5c)
Attachment #574478 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 10

6 years ago
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).
Attachment #575984 - Flags: review?(jwalden+bmo)
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.
Attachment #575981 - Flags: review?(sphink) → review+
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.
Attachment #575984 - Flags: review?(jwalden+bmo) → review+
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.
(Assignee)

Comment 14

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

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/de66e7bd2b98
https://hg.mozilla.org/mozilla-central/rev/de66e7bd2b98
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Created attachment 582553 [details] [diff] [review]
Get at the JSContext through IDL
Attachment #582553 - Flags: review?(bobbyholley+bmo)
Attachment #582553 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 582553 [details] [diff] [review]
Get at the JSContext through IDL

https://hg.mozilla.org/mozilla-central/rev/25f20780aa23
Attachment #582553 - Flags: checkin+

Comment 19

5 years ago
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
Keywords: dev-doc-needed
Depends on: 716702
(Assignee)

Updated

5 years ago
Blocks: 771118
You need to log in before you can comment on or make changes to this bug.