Remove unused marking code from JIT code map

Assigned to



JavaScript: GC
7 months ago
a month ago


(Reporter: jonco, Assigned: jonco)



Firefox Tracking Flags

(Not tracked)



(1 attachment)



7 months ago
There's a whole bunch if isMarkedFromAnyThread methods here that don't seem to be called from anywhere.

Comment 1

7 months ago
Created attachment 8853021 [details] [diff] [review]

Remove unused code.
Attachment #8853021 - Flags: review?(kvijayan)
Yeah, I have a patch for that too. Thought I'm having second thoughts about it.

What appears to be going on is that the marking depends on some complex and fragile-sounding assumptions, and that dead code was meant for testing whether the assumptions held. But it looks like the test must have broken. I haven't looked at blame, but I'm guessing that's what this comment is referring to:

    // JitcodeGlobalEntries are marked at the end of the mark phase. A read
    // barrier is not needed. Any JS frames sampled during the sweep phase of
    // the GC must be on stack, and on-stack frames must already be marked at
    // the beginning of the sweep phase. It's not possible to assert this here
    // as we may not be off thread when called from the gecko profiler.

Or in more detail:

    // JitcodeGlobalTable must keep entries that are in the sampler buffer
    // alive. This conditionality is akin to holding the entries weakly.
    // If this table were marked at the beginning of the mark phase, then
    // sampling would require a read barrier for sampling in between
    // incremental GC slices. However, invoking read barriers from the sampler
    // is wildly unsafe. The sampler may run at any time, including during GC
    // itself.
    // Instead, JitcodeGlobalTable is marked at the beginning of the sweep
    // phase, along with weak references. The key assumption is the
    // following. At the beginning of the sweep phase, any JS frames that the
    // sampler may put in its buffer that are not already there at the
    // beginning of the mark phase must have already been marked, as either 1)
    // the frame was on-stack at the beginning of the sweep phase, or 2) the
    // frame was pushed between incremental sweep slices. Frames of case 1)
    // are already marked. Frames of case 2) must have been reachable to have
    // been newly pushed, and thus are already marked.
    // The approach above obviates the need for read barriers. The assumption
    // above is checked in JitcodeGlobalTable::lookupForSampler.

That last part is now a lie. I'm guessing that was originally what the dead code was doing.

And yet, it seems like it would be a really good thing to test that assumption. Is there a way to do that?

Comment 3

7 months ago
Comment on attachment 8853021 [details] [diff] [review]

Cancelling review until we work out whether we want to keep this code around.
Attachment #8853021 - Flags: review?(kvijayan)
Keywords: triage-deferred
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.