We should be able to remove one word from every object, if we can remove the inheritorID.
Created attachment 140652 [details] Needs debugging - Causes GC crashiness in layouttests.
Created attachment 141104 [details] Fix No performance impact.
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.
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.
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.
(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...
Fixed in r116670
This caused a regression on an internal Apple site, <rdar://problem/11434001>.
And iCloud.com, <rdar://problem/11434252>. Roll out?
It's not rolling out easily. Please take a look.
(In reply to comment #10) > It's not rolling out easily. Please take a look. Will do.