[webkit-reviews] review granted: [Bug 85853] Cache inheritorID on JSFunction : [Attachment 141104] Fix

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 85853: Cache inheritorID on JSFunction
https://bugs.webkit.org/show_bug.cgi?id=85853

Attachment 141104: Fix
https://bugs.webkit.org/attachment.cgi?id=141104&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.