Last Comment Bug 723313 - Stop using conservative stack scanner for VM stack marking
: Stop using conservative stack scanner for VM stack marking
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Bill McCloskey (:billm)
:
Mentors:
: 714619 (view as bug list)
Depends on: 727921 728086 728190 732087 738841 738846 747926 749039
Blocks: 716647
  Show dependency treegraph
 
Reported: 2012-02-01 15:01 PST by Bill McCloskey (:billm)
Modified: 2012-09-07 13:22 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.24 KB, patch)
2012-02-01 15:01 PST, Bill McCloskey (:billm)
bhackett1024: review+
Details | Diff | Splinter Review
patch v2 (4.68 KB, patch)
2012-02-01 19:07 PST, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-02-01 15:01:35 PST
Created attachment 593618 [details] [diff] [review]
patch

This is part of the long-term goal of conservative scanner removal. But I also need it to test incremental GC.
Comment 1 Brian Hackett (:bhackett) 2012-02-01 15:41:46 PST
Comment on attachment 593618 [details] [diff] [review]
patch

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

Looks good, though there is another problem I remembered where a script's analysis info can be cleared without triggering a GC, if the compartment's debug mode is changed.  See JSCompartment::updateForDebugMode.  Torn values might not then be cleared by the GC here, if debug mode is turned on with optimized frames on the stack.  Fortunately, that situation is currently impossible, so nothing needs to be done for this patch, but will become possible with bug 716647 so it would be good to ensure things don't subtly break after both land.

Ultimately bug 716647 will want a stack walk similar to this in updateForDebugMode which clears out the locals with dead values when turning debug mode on.  Putting this stuff here as a note for that bug.
Comment 2 Bill McCloskey (:billm) 2012-02-01 19:07:12 PST
Created attachment 593703 [details] [diff] [review]
patch v2

This fixes a bug where it wasn't possible to get a StackSegment's regs().
Comment 3 Bill McCloskey (:billm) 2012-02-02 10:02:45 PST
Comment on attachment 593703 [details] [diff] [review]
patch v2

This is failing on Tinderbox.
Comment 4 Bill McCloskey (:billm) 2012-02-02 17:13:54 PST
Comment on attachment 593703 [details] [diff] [review]
patch v2

The failure was caused by an unrelated bug.
Comment 5 Luke Wagner [:luke] 2012-02-06 09:41:13 PST
Comment on attachment 593703 [details] [diff] [review]
patch v2

Nice patch.  I'm sorry for taking so long.
>+    if (!fp->isScriptFrame()) {
>+        gc::MarkRootRange(trc, slotsBegin, slotsEnd, "vm_stack");
>+        return;

Perhaps you already have found differently, but I would have thought you could JS_ASSERT(slotsBegin == slotsEnd).  Also JS_ASSERT(fp->isDummyFrame()) would be nice to explain why.

>+    if (!script->hasAnalysis() || !script->analysis()->ranLifetimes()) {
>+        gc::MarkRootRange(trc, slotsBegin, slotsEnd, "vm_stack");

If a shrinking gc throws away analysis results

>+     * slots considered not live. We need to avoid marking them. Additionally,
>+     * in case the analysis information is thrown out later, we overwrite these
>+     * dead slots with valid values so that future GCs won't crash.

Perhaps append "Analysis results are thrown away during the sweeping phase, so we always have at least one GC to do this." (unless you think its trivial :) ?

>+        jsbytecode *pc = seg->maybeRegs() ? seg->regs().pc : NULL;

Could you add/use seg->maybepc() similar to seg->maybefp() ?

>+            /* Mark from fp->slots() to slotsEnd. */
>+            markFrame(trc, fp, slotsEnd, pc);
>+
>             js_TraceStackFrame(trc, fp);

Perhaps markFrameSlots then?  (And, if you feel like it, s/js_TraceStackFrame/StackFrame::mark/ (and moved to Stack.cpp...))
Comment 6 Luke Wagner [:luke] 2012-02-06 09:42:25 PST
Ignore the "If a shrinking gc throws away" line, it was interrupted and ultimately supplanted by the following line :)
Comment 8 Phil Ringnalda (:philor) 2012-02-10 19:51:14 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/71f5bf4df2f6 - one of the six in that push was crashing in js::gc::Mark<JSString>
Comment 10 Marco Bonardo [::mak] 2012-02-13 09:06:22 PST
https://hg.mozilla.org/mozilla-central/rev/fbef6a165cf8
Comment 11 Bill McCloskey (:billm) 2012-05-04 11:09:54 PDT
*** Bug 714619 has been marked as a duplicate of this bug. ***

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