Last Comment Bug 785268 - IonMonkey: Improve style/design of profiling
: IonMonkey: Improve style/design of profiling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Alex Crichton [:acrichto]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-23 16:49 PDT by Alex Crichton [:acrichto]
Modified: 2012-08-24 10:05 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (23.83 KB, patch)
2012-08-23 16:49 PDT, Alex Crichton [:acrichto]
no flags Details | Diff | Review
patch (v2) (26.85 KB, patch)
2012-08-23 17:04 PDT, Alex Crichton [:acrichto]
dvander: review+
Details | Diff | Review

Description Alex Crichton [:acrichto] 2012-08-23 16:49:55 PDT
Created attachment 654848 [details] [diff] [review]
patch
Comment 1 Alex Crichton [:acrichto] 2012-08-23 17:04:02 PDT
Created attachment 654853 [details] [diff] [review]
patch (v2)
Comment 2 David Anderson [:dvander] 2012-08-23 18:20:25 PDT
Comment on attachment 654853 [details] [diff] [review]
patch (v2)

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

Thanks for doing this!

::: js/src/ion/IonInstrumentation.h
@@ +20,5 @@
> +
> +class IonInstrumentation : public BaseInstrumentation
> +{
> +    jsbytecode **trackedPc_;
> +  public:

nit: newline above public:

::: js/src/ion/IonMacroAssembler.h
@@ +523,5 @@
> +        return ret;
> +    }
> +
> +    // see above comment for what is returned
> +    uint32 callWithExitFrame(IonCode *target, Register dynStack) {

Yay, this seems a lot better.

@@ +535,5 @@
>    private:
> +    // These two functions are helpers used around call sites throughout the
> +    // assembler. They are called from the above call wrappers to emit the
> +    // necessary instrumentation.
> +    void leaveBeforeCall() {

These names are a little vague, it's not clear what "leave" or "call" refers to. Maybe something like  enter/ExitSpsFrame or enter/leaveProfilingFrame?

@@ +539,5 @@
> +    void leaveBeforeCall() {
> +        if (!sps_ || !sps_->enabled())
> +            return;
> +        GeneralRegisterSet regs(Registers::TempMask);
> +        Register r = regs.getAny();

I would just use ReturnReg or CallTempReg0 here, or:

Register r = GeneralRegisterSet(Registers::TempMask).getAny();

@@ +552,5 @@
> +        GeneralRegisterSet regs(Registers::TempMask & ~Registers::JSCallMask &
> +                                                      ~Registers::CallMask);
> +        if (regs.empty()) {
> +            regs = GeneralRegisterSet(Registers::TempMask);
> +            Register r = regs.getAny();

Same here.
Comment 3 Alex Crichton [:acrichto] 2012-08-23 18:37:49 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/60b7209fbe71

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