Closed
Bug 790836
Opened 12 years ago
Closed 12 years ago
Don't mark IonJSFrameLayout twice
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: terrence, Assigned: terrence)
References
Details
(Whiteboard: [js:t])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
When marking the stack conservatively, we skip regions that are owned by Ion: it marks its stack exactly at a different time. When copying these exclusions into the rooting analysis -- we were incorrectly poisoning IonMonkey's stack -- Sean noticed that the top we are using for the exclusion does not include the IonJSFrameLayout. This causes us to mark any objects in the Layout twice. This is perfectly fine when marking, but messes up the rooting analysis.
Attachment #660650 -
Flags: review?(nicolas.b.pierron)
Comment 1•12 years ago
|
||
Comment on attachment 660650 [details] [diff] [review]
v0
Review of attachment 660650 [details] [diff] [review]:
-----------------------------------------------------------------
Good catch, still not correctly fixed yet.
::: js/src/jsgc.cpp
@@ +1173,5 @@
> ion::IonFrameIterator frames(ion.top());
> while (!frames.done())
> ++frames;
>
> uintptr_t *ionMin = (uintptr_t *)ion.top();
We recently added a footer to the exit frame. Everything in the footer should be marked precisely.
ion::IonFrameIterator frames(ion.top());
uintptr_t *ionMin = (uintptr_t *) frame.exitFrame()->ionStackBegin();
while (!frames.done())
++frames;
This new function should return « frame.exitFrame()->footer() » when the exit frame is not a VMWrapper or if it does not have an out param. otherwise it should return « frame.exitFrame()->footer()->outVp() ». Have a look at MarkIonExitFrame (from ionFrames.cpp) to make sure you are doing it right.
@@ +1174,5 @@
> while (!frames.done())
> ++frames;
>
> uintptr_t *ionMin = (uintptr_t *)ion.top();
> + uintptr_t *ionEnd = (uintptr_t *)frames.fp() + sizeof(ion::IonJSFrameLayout);
This is still wrong and won't change a thing except that you won't mark the script but you will still mark the arguments of the JS function twice. Use the following instead:
uintptr_t *ionEnd = (uintptr_t *) frames.prevFp();
You will have to remove/specialize the assertion inside prevFp such as you can use it here. It should be safe to remove it now.
@@ +4969,5 @@
> + while (!frames.done())
> + ++frames;
> +
> + uintptr_t *ionMin = (uintptr_t *)ion.top();
> + uintptr_t *ionEnd = (uintptr_t *)frames.fp() + sizeof(ion::IonJSFrameLayout);
idem. You might want to promote this computation to a method of IonActivationFrameIterator, such as
IonActivationFrameIterator::stackRange(uintptr_t &start, uintptr_t &end) const;
and add it near the marking functions, such as we can relate to it.
Attachment #660650 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•12 years ago
|
||
With fixes applied.
Attachment #660650 -
Attachment is obsolete: true
Attachment #661044 -
Flags: review?(nicolas.b.pierron)
Comment 3•12 years ago
|
||
Comment on attachment 661044 [details] [diff] [review]
v1
Review of attachment 661044 [details] [diff] [review]:
-----------------------------------------------------------------
Apply nits, and r=me.
::: js/src/ion/IonFrames.cpp
@@ +491,5 @@
> #endif
> }
>
> +void
> +IonFrameIterator::ionStackRange(uintptr_t *&min, uintptr_t *&end)
nit: move it to the IonActivationIterator, otherwise the meaning would be the stack range of one Ion frame.
@@ +495,5 @@
> +IonFrameIterator::ionStackRange(uintptr_t *&min, uintptr_t *&end)
> +{
> + IonExitFooterFrame *footer = exitFrame()->footer();
> + const VMFunction *f = footer->function();
> + if (f == NULL || f->outParam != Type_Handle)
nit: if (exitFrame()->nativeExit() || exitFrame()->DOMExit() || f->outParam != Type_Handle)
Attachment #661044 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Hurm. |if (exitFrame()->nativeExit() || exitFrame()->DOMExit() || f->outParam != Type_Handle)| trips all the assertions. After discussing it with Sean, I think we don't need to test the frame type, just whether vp exists. We always want to skip it, regardless of frame type. What we came up with is:
if (f && f->outParam == Type_Handle)
min = reinterpret_cast<uintptr_t *>(footer->outVp());
else
min = reinterpret_cast<uintptr_t *>(footer);
Do you concur?
Attachment #661044 -
Attachment is obsolete: true
Attachment #661412 -
Flags: review?(nicolas.b.pierron)
Comment 5•12 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #4)
> Created attachment 661412 [details] [diff] [review]
> v2
>
> Hurm. |if (exitFrame()->nativeExit() || exitFrame()->DOMExit() ||
> f->outParam != Type_Handle)| trips all the assertions. After discussing it
> with Sean, I think we don't need to test the frame type, just whether vp
> exists. We always want to skip it, regardless of frame type. What we came
> up with is:
>
> if (f && f->outParam == Type_Handle)
> min = reinterpret_cast<uintptr_t *>(footer->outVp());
> else
> min = reinterpret_cast<uintptr_t *>(footer);
>
> Do you concur?
I agree this would work, but this is likely to be missed by later modifications of the footer unless you can add a comment mentioning this location if this really needs to be optimized.
the best would be:
if (exitFrame()->wrapperExit() && …)
where wrapperExit() is an inlined function which coerces f to a boolean.
Comment 6•12 years ago
|
||
Comment on attachment 661412 [details] [diff] [review]
v2
Review of attachment 661412 [details] [diff] [review]:
-----------------------------------------------------------------
See previous comment, apply if possible and r=me.
Attachment #661412 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 7•12 years ago
|
||
Comment 8•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•