679136 - JSOPTION_PCCOUNT should use js::Interpret's interrupts to count opcodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Attachment: Use js::Interpret's interrupts to implement JSOPTION_PCCOUNT's bytecode profiling.

At the moment, the DO_OP macro in js::Interpret calls COUNT_OP, which expands to:

JS_BEGIN_MACRO                                     \
    if (pcCounts && !regs.fp()->hasImacropc())     \
        ++pcCounts[regs.pc - script->code];        \
JS_END_MACRO

It doesn't seem ideal to put a branch on the hot path of every opcode dispatch. Instead, when the script has a pcCounts table, the interpreter should use the interruptJumpTable, and the 'interrupt:' path should count the opcodes.

Comment 1

[Jim Blandy :jimb]

Attachment: Use js::Interpret's interrupts to implement JSOPTION_PCCOUNT's bytecode profiling.

Improved macro names.
Pulled "pcCount -> interrupts enabled" assertion out into a macro, made it check for both threaded and non-threaded interpreters.

Comment 2

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

Comment on attachment 553268 [details] [diff] [review]
Use js::Interpret's interrupts to implement JSOPTION_PCCOUNT's bytecode profiling.

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

I really wish I had thought of this approach. It's much, much better than the original, and obsoletes my current patch to add configure flags and variant outputs to avoid the perf impact.

Patch looks great to me.

Comment 3

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #2)
> Comment on attachment 553268 [details] [diff] [review]
> Use js::Interpret's interrupts to implement JSOPTION_PCCOUNT's bytecode
> profiling.
> 
> Review of attachment 553268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really wish I had thought of this approach. It's much, much better than
> the original, and obsoletes my current patch to add configure flags and
> variant outputs to avoid the perf impact.
> 
> Patch looks great to me.

I'm glad I could make it work!

Comment 4

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8391d102fd70

Comment 5

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/8391d102fd70