Last Comment Bug 679136 - JSOPTION_PCCOUNT should use js::Interpret's interrupts to count opcodes
: JSOPTION_PCCOUNT should use js::Interpret's interrupts to count opcodes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on:
Blocks: onStep
  Show dependency treegraph
 
Reported: 2011-08-15 14:19 PDT by Jim Blandy :jimb
Modified: 2011-08-26 09:35 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use js::Interpret's interrupts to implement JSOPTION_PCCOUNT's bytecode profiling. (3.08 KB, patch)
2011-08-15 14:19 PDT, Jim Blandy :jimb
no flags Details | Diff | Review
Use js::Interpret's interrupts to implement JSOPTION_PCCOUNT's bytecode profiling. (3.53 KB, patch)
2011-08-15 14:37 PDT, Jim Blandy :jimb
sphink: review+
Details | Diff | Review

Description Jim Blandy :jimb 2011-08-15 14:19:33 PDT
Created attachment 553261 [details] [diff] [review]
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 2011-08-15 14:37:34 PDT
Created attachment 553268 [details] [diff] [review]
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:] 2011-08-15 14:44:44 PDT
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 2011-08-15 20:09:34 PDT
(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] 2011-08-25 15:08:43 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8391d102fd70
Comment 5 Matt Brubeck (:mbrubeck) 2011-08-26 09:35:47 PDT
http://hg.mozilla.org/mozilla-central/rev/8391d102fd70

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