Closed Bug 679136 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #553261 - Flags: review?(sphink)
Improved macro names.
Pulled "pcCount -> interrupts enabled" assertion out into a macro, made it check for both threaded and non-threaded interpreters.
Attachment #553261 - Attachment is obsolete: true
Attachment #553261 - Flags: review?(sphink)
Attachment #553268 - Flags: review?(sphink)
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.
Attachment #553268 - Flags: review?(sphink) → review+
(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!
Blocks: onStep
http://hg.mozilla.org/mozilla-central/rev/8391d102fd70
Assignee: general → jimb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.