DFG does not support compiling functions as constructors

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

The optimizing compiler in JavaScriptCore can't compile constructors.  That should be fixed.

[Filip Pizlo]

Created attachment 108234 [details]
the patch

[Geoffrey Garen]

Comment on attachment 108234 [details]
the patch

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1538
> +        SpeculateCellOperand proto(this, node.child1());

'this' is always a cell. Does the DFG know that? Something good to fix in a follow-up patch, if not. (SpeculateCellOperand here is fine, but you should make sure the branch gets elided under the covers.)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1563
> +        // Need to verify that the prototype is an object. If we have reason to believe
> +        // that it's a FinalObject then we speculate on that directly. Otherwise we
> +        // do the slow (structure-based) check.
> +        if (shouldSpeculateFinalObject(node.child1()))
> +            speculationCheck(m_jit.branchPtr(MacroAssembler::NotEqual, MacroAssembler::Address(protoGPR), MacroAssembler::TrustedImmPtr(m_jit.globalData()->jsFinalObjectVPtr)));
> +        else {
> +            m_jit.loadPtr(MacroAssembler::Address(protoGPR, JSCell::structureOffset()), scratchGPR);
> +            slowPath.append(m_jit.branch8(MacroAssembler::Below, MacroAssembler::Address(scratchGPR, Structure::typeInfoTypeOffset()), MacroAssembler::TrustedImm32(ObjectType)));
> +        }
> +        
> +        // Load the inheritorID (the Structure that objects who have protoGPR as the prototype
> +        // use to refer to that prototype). If the inheritorID is not set, go to slow path.
> +        m_jit.loadPtr(MacroAssembler::Address(protoGPR, JSObject::offsetOfInheritorID()), scratchGPR);
> +        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, scratchGPR));

You could remove all of this loading and  branching in a follow-up patch if you made assignments to function.prototype eagerly jettison the function's constructor code.

[Filip Pizlo]

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1538
> > +        SpeculateCellOperand proto(this, node.child1());
> 
> 'this' is always a cell. Does the DFG know that? Something good to fix in a follow-up patch, if not. (SpeculateCellOperand here is fine, but you should make sure the branch gets elided under the covers.)

The code you're referring to is speculating that the function.prototype is a cell.  Is that guaranteed?

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1563
> > +        // Need to verify that the prototype is an object. If we have reason to believe
> > +        // that it's a FinalObject then we speculate on that directly. Otherwise we
> > +        // do the slow (structure-based) check.
> > +        if (shouldSpeculateFinalObject(node.child1()))
> > +            speculationCheck(m_jit.branchPtr(MacroAssembler::NotEqual, MacroAssembler::Address(protoGPR), MacroAssembler::TrustedImmPtr(m_jit.globalData()->jsFinalObjectVPtr)));
> > +        else {
> > +            m_jit.loadPtr(MacroAssembler::Address(protoGPR, JSCell::structureOffset()), scratchGPR);
> > +            slowPath.append(m_jit.branch8(MacroAssembler::Below, MacroAssembler::Address(scratchGPR, Structure::typeInfoTypeOffset()), MacroAssembler::TrustedImm32(ObjectType)));
> > +        }
> > +        
> > +        // Load the inheritorID (the Structure that objects who have protoGPR as the prototype
> > +        // use to refer to that prototype). If the inheritorID is not set, go to slow path.
> > +        m_jit.loadPtr(MacroAssembler::Address(protoGPR, JSObject::offsetOfInheritorID()), scratchGPR);
> > +        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, scratchGPR));
> 
> You could remove all of this loading and  branching in a follow-up patch if you made assignments to function.prototype eagerly jettison the function's constructor code.

That would be interesting.  It would have to be done carefully, like making sure that there is no way any other JS code can execute in the prologue of the constructor.  I think that this should be trivially true once you know that .prototype is not insane.

For now though, I'm going for softer targets.  Like all of the insanity on the put_by_id's that follow create_this.  We don't do anything special for them right now in DFG.

[Filip Pizlo]

Landed in r95672.

[Ryosuke Niwa]

It appears that this patch caused hundreds of tests to fail:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33283

[Ryosuke Niwa]

It appears that this patch caused hundreds of tests to fail:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/33283

[Ryosuke Niwa]

I'm sorry but I have to roll out this patch for now. We're losing the entire test coverage due to this failure.