[Webkit-unassigned] [Bug 72312] DFG code blocks that have speculation checks on objects should refer to those objects weakly

Thu Nov 17 14:03:29 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=72312

--- Comment #8 from Filip Pizlo <fpizlo at apple.com>  2011-11-17 14:03:29 PST ---
(In reply to comment #7)
> (From update of attachment 115538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115538&action=review
> 
> Thinking abstractly, I'm not sure this is the right strategy for making optimizations weakly referenced. But perhaps there's a concrete application I'm not considering, which validates this strategy. I would have a much easier time reasoning about this code if I could look at a benchmark or test case that demonstrated the problem we're trying to solve.

// One place in your code:
function foo(f) {
    return f(42);
}

// Another place in your code:
function bar() {
    foo(someWindow.baz);
}

There's a good chance t hat someWindow.baz will be inlined into foo().  Now so long as foo() is alive, someWindow is alive.

> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1623
> > +    // Add a weak reference harvester if we have not reached fixpoint and need to
> > +    // run again.
> 
> To turn this into a "why" comment, I would write: GC doesn't have enough information yet for us to decide whether to keep our DFG data, so we need to register a handler to run again at the end of GC, when more information is available.
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1657
> > +                // If the following three things are live, then the target of the
> > +                // transition is also live:
> > +                // - This code block. We know it's live already because otherwise
> > +                //   we wouldn't be scanning ourselves.
> > +                // - The code origin of the transition. Transitions may arise from
> > +                //   code that was inlined. They are not relevant if the user's
> > +                //   object that is required for the inlinee to run is no longer
> > +                //   live.
> > +                // - The source of the transition. The transition checks if some
> > +                //   heap location holds the source, and if so, stores the target.
> > +                //   Hence the source must be live for the transition to be live.
> 
> Why do we want structure transitions to perform checks on behalf of inlined functions? It would be much more natural for functions themselves to act as first-class weak citizens, such that a linked/inlined function disappearing would be sufficient cause to blow away an optimized code block.

They don't do anything on behalf of functions.  Functions are also separately weak referenced.  Transitions just honor the fact that the transition isn't going to happen unless the owner is alive.

> 
> More broadly, I don't understand why structure transitions get special treatment here. Why are they distinct from other weakly referenced optimizations in a DFG code block?

Say you have Foo used as constructor:

function Foo() {
   this.a = 1;
   this.b = 2;
   this.c = 3;
}

If during GC, the structure corresponding to empty->a->b->c may not be reachable from anywhere but Foo(), because Foo() hadn't run recently, or did, but it's result was short-lived.  But Foo() is alive.  So we want to keep empty->a->b->c alive as well, because otherwise, we'd have to throw away Foo().

Since Foo() may only exist in inlined form, the inline owner must look at the structure transitions of each of these assignments and check if both the owner (Foo) is alive, and the predecessor in the transition is alive, before deciding that the successor is alive.

> 
> I'm concerned that an A->B transition keeps B alive if A is alive. This is the opposite of the structure marking strategy, which says that A should stay alive only if B is alive. The benefit of the structure marking strategy is that it ensures that a chain of structures can be retired if it becomes stale. In the case of object literals, A is the default empty object structure, which is permanent, so, with this patch's strategy, all transitions from an object literal are permanent. In the case of a constructor function, A is the function's .prototype's inheritorID, which will have been marked by the same object marking the CodeBlock, so, with this patch's strategy, the transition once again becomes permanent. (To clarify, by "permanent" I mean, "will live as long as a strong reference", not necessarily "memory leak".)

No, they won't be permanent.  Because the prerequisite to the A->B transition keeping B alive is that the code owner is also alive.

> 
> Maybe you're trying to make sure that a hot constructor CodeBlock survives even if the objects it constructs tend to be garbage? If so, I'm still not sure why the transitions are special compared to other weakly optimized assumptions. It seems like any weakly optimized assumption could pertain to a typically short-lived object, and cause jettison churn. Perhaps a better strategy would be an explicit jettison churn guard, instead.

Not sure I follow.

The transition logic is not about making CodeBlocks survive.  It's about making the transitions they use remain valid.  Then separately the CodeBlock will survive if its weak references survive, and some of its weak references will be from its transitions (all of the values in WeakReferenceTransition are also in the CodeBlock's weak reference list).

> 
> > Source/JavaScriptCore/bytecode/CodeBlock.h:1111
> > +            // Am I a DFG code block? If not, then I'm live if I am being scanned.
> > +            if (!m_dfgData)
> > +                return true;
> > +            
> > +            // If I am a DFG code block, then am I currently executing? If so,
> > +            // then I'm definitely live.
> > +            if (m_dfgData->mayBeExecuting)
> 
> I don't think these "what" comments add anything. A "why" comment explaining why baseline JIT code always assumes itself to be live might be helpful.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.