Closed
Bug 785268
Opened 13 years ago
Closed 13 years ago
IonMonkey: Improve style/design of profiling
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: u443197, Assigned: u443197)
Details
Attachments
(1 file, 1 obsolete file)
26.85 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #654848 -
Attachment is obsolete: true
Attachment #654853 -
Flags: review?(dvander)
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.
Attachment #654853 -
Flags: review?(dvander) → review+
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•