Bug 85853 – Cache inheritorID on JSFunction

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

We should be able to remove one word from every object, if we can remove the inheritorID.

[Gavin Barraclough]

Created attachment 140652 [details]
Needs debugging - Causes GC crashiness in layouttests.

[Gavin Barraclough]

Created attachment 141104 [details]
Fix

No performance impact.

[WebKit Review Bot]

Attachment 141104 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:461:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 141104 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=141104&action=review

Seems fine to land this to move forward with nixing inheritorID, but see my two comments below -- I think there might be a better way to do this. Maybe discuss with Phil.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3042
> +        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, scratchGPR));

Would it be better just to treat a NULL inheritorID in a constructor as a spec failure? That would save on code size. The DFG should only see constructors that have already been called once.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3061
> +    case CreateThisInlined: {

I think you made inlining use a separate node type because:
(a) in the inline case, the callee can't be implicit based on call frame;
(b) in the non-inline case, SpeculateCellOperand would add a branch because the JIT doesn't know that the callee is always a JSCell.

Is that right?

Maybe I'm out of whack with the state of the art in our inlining, but I think it's kind of a shame to change node types based on inlining.

I'd prefer to see the callee always made an explicit node, with the JIT knowing the type of the callee register in a call frame. Then, we wouldn't need a separate node type depending on inlining, and inlining would work automatically based on virtual register remapping.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:461
>> +    Structure* structure = constructor->cachedInheritorID(exec  );
> 
> Extra space before )  [whitespace/parens] [2]

Fixo, please.

[Filip Pizlo]

Comment on attachment 141104 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=141104&action=review

r=me as well.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1538
> +                set(currentInstruction[1].u.operand, addToGraph(CreateThisInlined, getDirect(m_inlineStackTop->m_calleeVR)));

Just an observation: instead of getDirect() you could be putting a constant in here.  Probably makes no difference though, since at this point in the code the callee will be in a register anyway.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3042
>> +        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, scratchGPR));
> 
> Would it be better just to treat a NULL inheritorID in a constructor as a spec failure? That would save on code size. The DFG should only see constructors that have already been called once.

I'm not sure if it would save code size.  To speculate, we have to perform the nullcheck anyway, right?  And we already have a slow path.

So a speculation check instead of a slow path branch would only be beneficial if the CFA could propagate the proof generated by the null check to a subsequent use of the inheritor ID.  But I get the feeling that the inheritor ID is only used once.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3061
>> +    case CreateThisInlined: {
> 
> I think you made inlining use a separate node type because:
> (a) in the inline case, the callee can't be implicit based on call frame;
> (b) in the non-inline case, SpeculateCellOperand would add a branch because the JIT doesn't know that the callee is always a JSCell.
> 
> Is that right?
> 
> Maybe I'm out of whack with the state of the art in our inlining, but I think it's kind of a shame to change node types based on inlining.
> 
> I'd prefer to see the callee always made an explicit node, with the JIT knowing the type of the callee register in a call frame. Then, we wouldn't need a separate node type depending on inlining, and inlining would work automatically based on virtual register remapping.

I think we will be adding more of these types of things.  Inlining is special, because we typically have more information, and getting that information is cheaper!  Making a special node, in the current state of the DFG, means that we can create tighter code.

What does bug me about this code, though, is that it has to get the callee from somewhere instead of just using a constant.  But maybe that's OK.

[Gavin Barraclough]

(In reply to comment #5)
> Just an observation: instead of getDirect() you could be putting a constant in here.  Probably makes no difference though, since at this point in the code the callee will be in a register anyway.

* nod.  It is in a register, and I've left the code working as it was previously (for op_get_callee), so which leave as-if for now.  Lemme know if you're not happy with that resolution.

> So a speculation check instead of a slow path branch would only be beneficial if the CFA could propagate the proof generated by the null check to a subsequent use of the inheritor ID.  But I get the feeling that the inheritor ID is only used once.

Hmmm, seems worthwhile discussing further, and is a somewhat unrelated change anyway.  Will follow up offline to see if we want to file a bug to take further change here.

> > I'd prefer to see the callee always made an explicit node, with the JIT knowing the type of the callee register in a call frame. Then, we wouldn't need a separate node type depending on inlining, and inlining would work automatically based on virtual register remapping.
> 
> I think we will be adding more of these types of things.  Inlining is special, because we typically have more information, and getting that information is cheaper!  Making a special node, in the current state of the DFG, means that we can create tighter code.

Ugh, completely agreed in principal but I've just switched my code to use one version of CreateThis. :-)  There is 

> What does bug me about this code, though, is that it has to get the callee from somewhere instead of just using a constant.  But maybe that's OK.

In the non-inlined case, the callee isn't a constant because the function executable may be shared between inline functions (e.g. closures).  Of course, in the future it might be an interesting direction to evolve towards DFG code being JSFunction specific, in which case callee could be known constant...

[Gavin Barraclough]

Fixed in r116670

[Alexey Proskuryakov]

This caused a regression on an internal Apple site, <rdar://problem/11434001>.

[Alexey Proskuryakov]

And iCloud.com, <rdar://problem/11434252>. Roll out?

[Alexey Proskuryakov]

It's not rolling out easily. Please take a look.

[Gavin Barraclough]

(In reply to comment #10)
> It's not rolling out easily. Please take a look.

Will do.