Closed
Bug 810946
Opened 12 years ago
Closed 12 years ago
BaselineCompiler: Mark baseline frames
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Stores the frame size without VMFunction arguments in the frame, then uses that to mark all locals and stack values.
Attachment #680631 -
Flags: review?(kvijayan)
Assignee | ||
Comment 1•12 years ago
|
||
Fix a typo.
Attachment #680631 -
Attachment is obsolete: true
Attachment #680631 -
Flags: review?(kvijayan)
Attachment #680633 -
Flags: review?(kvijayan)
Comment 2•12 years ago
|
||
Comment on attachment 680633 [details] [diff] [review]
Patch v2
Review of attachment 680633 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/shared/IonFrames-x86-shared.h
@@ +450,5 @@
> + static const uint32_t FramePointerOffset = sizeof(void *);
> +
> + static inline size_t offsetOfFrameSize() {
> + return -offsetof(BaselineFrame, frameSize) - sizeof(uint32_t);
> + }
These offsets are intended to be used with reference to a pointer immediately above the BaselineFrame. At least that's what my reading of the marking code seems to indicate.
This expression may be clearer as:
-(sizeof(BaselineFrame) - offsetOf(BaselineFrame, frameSize))
And maybe we should call these method backwardsOffsetOf... or reverseOffsetOf... ? The offsetOf naming convention seems pretty solidly consistent to refer to the offset of things from a pointer to the start of a structure, not the end.
@@ +456,5 @@
> + return -(sizeof(BaselineFrame) + index * sizeof(js::Value)) - sizeof(js::Value);
> + }
> + static inline size_t offsetOfArg(size_t index) {
> + return FramePointerOffset + js::ion::IonJSFrameLayout::offsetOfActualArg(index);
> + }
The above statement about renaming the offsetOf... methods wouldn't apply to offsetOfArg since it does compute a forward offset.
Attachment #680633 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #681014 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 4•12 years ago
|
||
Pushed with the reverse* names + a comment.
https://hg.mozilla.org/projects/ionmonkey/rev/7c9cfaafa4a1
Comment 5•12 years ago
|
||
Comment on attachment 681014 [details] [diff] [review]
Dump baseline frames
Review of attachment 681014 [details] [diff] [review]:
-----------------------------------------------------------------
Sounds good. I am happy to see that adding the dumpBaseline function help clarify the bunch of JS_ASSERT of the Mark function.
Attachment #681014 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•