Support caching of custom setters
Created attachment 225506 [details] Patch
This does the right thing in the baseline jit, but crashes in the DFG. Still trying to work out why. My testcase is: function f() {} var o = RegExp; function test(o) { var k = 0; for (var i = 0; i < 10000; i++) { k += Number((o.input = ((i ^ k >> 12) & 5)) + o.input) // o.multiline = false; } return k; } var result test(o); for (var k = 0; k < 10; k++) { var start = new Date; var newResult = test(o) var end = new Date; print(newResult) if (k && newResult != result) throw "Failed at " + k + "with " +newResult + " vs. " + result result = newResult; print("** " + k + " levels took " + (end - start) + "ms"); o = {__proto__ : o } }
Attachment 225506 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/PutByIdVariant.h:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:199: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1168: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1208: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1266: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1269: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1270: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1271: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1314: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1393: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1394: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1395: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdVariant.cpp:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdVariant.cpp:57: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdVariant.cpp:58: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 225506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225506&action=review I think you need to remove all of your changes from PutByIdStatus and PutByIdVariant, and all of your changes to MultiPutByOffset. Those changes are completely wrong and also unnecessary. > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:107 > - || stubInfo.accessType == access_put_by_id_transition_direct); > + || stubInfo.accessType == access_put_by_id_transition_direct > + || stubInfo.accessType == access_unset); This change looks superfluous given that you're guarding calls to this with a check for access_unset. > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h:50 > + Custom Call it CustomSetter. > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h:91 > + static PutByIdAccess custom( call it customSetter > Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:199 > + access.oldStructure(), access.customSetter(), > + access.chain() ? adoptRef(new IntendedStructureChain( > + profiledBlock, access.oldStructure(), access.chain())) : 0)); This almost certainly needs to be indented an extra 4 spaces. > Source/JavaScriptCore/bytecode/PutByIdVariant.cpp:58 > + out.print( > + "<Custom: ", pointerDumpInContext(oldStructure(), context), " . ", > + pointerDumpInContext(structure() /* pretty ugly, but work around C++ cast rules*/ , context), "(), ", > + pointerDumpInContext(structureChain(), context), ", ", offset(), ">"); Really weird indentation here. Also, that comment is weird. What are you trying to work around here? If you're trying to get the custom setter function and print it, then congratulations, you've just created a really hard to debug crash. pointerDumpInContext(Structure*) will try to call Structure::dump, so if you're passing something that isn't a structure, you're creating a mess. > Source/JavaScriptCore/bytecode/PutByIdVariant.h:41 > + Custom Call it CustomSetter. > Source/JavaScriptCore/bytecode/PutByIdVariant.h:74 > + static PutByIdVariant custom(Structure* structure, PutPropertySlot::PutValueFunc customSetter, Call it customSetter. > Source/JavaScriptCore/bytecode/PutByIdVariant.h:75 > + PassRefPtr<IntendedStructureChain> structureChain) We're not doing emacs indentation in WebKit. > Source/JavaScriptCore/bytecode/PutByIdVariant.h:90 > + PutPropertySlot::PutValueFunc customSetter() const { ASSERT(m_kind == Custom); return m_customSetter; } Call it customSetterFunction() > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1678 > + > + if (variant.kind() == PutByIdVariant::Custom) { > + filter(node->child1(), structure); > + m_state.setFoundConstants(true); > + m_state.setHaveStructures(true); > + clobberWorld(node->origin.semantic, clobberLimit); > + done = true; > + break; > + } This makes no sense. What are you trying to do? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1706 > + if (variant.kind() == PutByIdVariant::Custom) { > + if (value.m_currentKnownStructure.contains(variant.structure())) > + newSet.addAll(variant.structure()); > + continue; > + } What? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1774 > + if (status[0].kind() == PutByIdVariant::Custom) { > + m_state.setHaveStructures(true); > + m_state.setFoundConstants(true); > + clobberWorld(node->origin.semantic, clobberLimit); > + break; > + } Again, zero sense. Why is this here. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1965 > + if (putByIdStatus.numVariants() > 1 || putByIdStatus[0].kind() == PutByIdVariant::Custom) { Huh? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1970 > - > + CRASH(); What? > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:685 > + if (node->multiPutByOffsetData().hasCustom()) > + return 0; Why? > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:738 > + if (node->multiPutByOffsetData().hasCustom()) > + return 0; Why? > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:809 > + if (node->multiPutByOffsetData().hasCustom()) > + return 0; Why? > Source/JavaScriptCore/dfg/DFGClobberize.h:503 > + if (node->multiPutByOffsetData().hasCustom()) { > + read(World); > + write(World); > + } This is really wrong. > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:192 > + if (variant.kind() == PutByIdVariant::Custom) > + continue; OK. This really makes no sense. Why does the abstract interpreter claim to have found a constant when it sees PutByIdVariant::Custom, only so that constant folding can reject PutByIdVariant::Custom? > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:250 > - if (status.numVariants() != 1) > + if (status.numVariants() != 1 && status[0].kind() == PutByIdVariant::Custom) I suspect that you meant || here, but also, this is just wrong. > Source/JavaScriptCore/ftl/FTLCapabilities.cpp:149 > + > + case MultiPutByOffset: > + if (node->multiPutByOffsetData().hasCustom()) > + return CannotCompile; > + break; > + Why are you creating MultiPutByOffsets with custom setters only to reject them? This means that any code that calls custom setters will no longer compile with the FTL, whereas previous to your change, it would have compiled in the FTL just fine.
(In reply to comment #4) > (From update of attachment 225506 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225506&action=review > > I think you need to remove all of your changes from PutByIdStatus and PutByIdVariant, and all of your changes to MultiPutByOffset. Those changes are completely wrong and also unnecessary. > > > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:107 > > - || stubInfo.accessType == access_put_by_id_transition_direct); > > + || stubInfo.accessType == access_put_by_id_transition_direct > > + || stubInfo.accessType == access_unset); > > This change looks superfluous given that you're guarding calls to this with a check for access_unset. > > > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h:50 > > + Custom > > Call it CustomSetter. > > > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h:91 > > + static PutByIdAccess custom( > > call it customSetter Righto Indentation comments elided as they're just Xcode attempting to auto indent and i'm aware of the current mess :D > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1678 > > + > > + if (variant.kind() == PutByIdVariant::Custom) { > > + filter(node->child1(), structure); > > + m_state.setFoundConstants(true); > > + m_state.setHaveStructures(true); > > + clobberWorld(node->origin.semantic, clobberLimit); > > + done = true; > > + break; > > + } > > This makes no sense. What are you trying to do? > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1706 > > + if (variant.kind() == PutByIdVariant::Custom) { > > + if (value.m_currentKnownStructure.contains(variant.structure())) > > + newSet.addAll(variant.structure()); > > + continue; > > + } > > What? > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1774 > > + if (status[0].kind() == PutByIdVariant::Custom) { > > + m_state.setHaveStructures(true); > > + m_state.setFoundConstants(true); > > + clobberWorld(node->origin.semantic, clobberLimit); > > + break; > > + } > > Again, zero sense. Why is this here. > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1965 > > + if (putByIdStatus.numVariants() > 1 || putByIdStatus[0].kind() == PutByIdVariant::Custom) { > > Huh? > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1970 > > - > > + CRASH(); > > What? glorified printf debugging :D > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:685 > > + if (node->multiPutByOffsetData().hasCustom()) > > + return 0; > > Why? > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:738 > > + if (node->multiPutByOffsetData().hasCustom()) > > + return 0; > > Why? > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:809 > > + if (node->multiPutByOffsetData().hasCustom()) > > + return 0; > > Why? > If we're calling a setter arbitrary code may execute so you can't CSE across them. Though now that i think about it that's clearly stupid - we just know we can't CSE a setter, and the rest of the node data will ensure we don't try ro reuse values that may be mutated across the call. > > Source/JavaScriptCore/dfg/DFGClobberize.h:503 > > + if (node->multiPutByOffsetData().hasCustom()) { > > + read(World); > > + write(World); > > + } > > This is really wrong. Why? a custom setter can run arbitrary code so can read/write anything > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:192 > > + if (variant.kind() == PutByIdVariant::Custom) > > + continue; > > OK. This really makes no sense. Why does the abstract interpreter claim to have found a constant when it sees PutByIdVariant::Custom, only so that constant folding can reject PutByIdVariant::Custom? *shrug* > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:250 > > - if (status.numVariants() != 1) > > + if (status.numVariants() != 1 && status[0].kind() == PutByIdVariant::Custom) > > I suspect that you meant || here, but also, this is just wrong. This && should be ||, as i was just trying to minimise code i was writing until i had the basics working > > > Source/JavaScriptCore/ftl/FTLCapabilities.cpp:149 > > + > > + case MultiPutByOffset: > > + if (node->multiPutByOffsetData().hasCustom()) > > + return CannotCompile; > > + break; > > + > > Why are you creating MultiPutByOffsets with custom setters only to reject them? This means that any code that calls custom setters will no longer compile with the FTL, whereas previous to your change, it would have compiled in the FTL just fine. Again, minimising the working surface to just the DFG while i work out what the DFG should be doing.
The main problem with this patch is that it tries to lie to the DFG/FTL PutById type inference code. It tries to tell the code that CustomSetters are inlineable. It appears that you are trying to then actually inline them, by emitting MultiPutByOffsets for them. And then you're telling the FTL that MultiPutByOffsets aren't supported. Since MultiPutByOffsets are only emitted on the FTL path, that just means that if we have a function that uses custom setters then: - The DFG will inject execution count profiling under the assumption that this function will be an FTL candidate. This will make the DFG code run slower. - The DFG->FTL OSR code will fire eventually and it will try to run the FTL. - The FTL will tell the DFG bytecode parser to emit MultiPutByOffsets with CustomSetter variants. - At the bitter end of the DFG optimization pipeline, we'll realize that the MultiPutByOffsets contain a CustomSetter, so we will bail from FTL compilation. - The function will then most likely be stuck running slow DFG code - slower than ordinary DFG code because it still has profiling for entry to the FTL. So, in that regard, that part of the patch is a regression. On top of that, I don't think that your treatment of MultiPutByOffsets and PutByIds is right. You're making MultiPutByOffset be a possibly-clobber-the-world node, but you're neglecting to mark it as either NodeClobbersWorld or NodeMightClobber and you're not giving it any treatment in the Graph methods for detecting the clobberyness of nodes. It's good that you did the right things in clobberize() but right now that's not the only place that needs to know. Also, it's wrong to claim that you found constants in the abstract interpreter, if you have no intention to have the constant folder do anything. It appears that you're doing this. It'll just be a compile-time regression. The correct way to add this kind of functionality to the DFG and FTL is to do nothing. Keep all of the functionality in Repatch, StructureStubInfo, and PolymorphicPutByIdList. The DFG and FTL will magically get all of this functionality by virtue of the custom setter call being turned into a PutById and that PutById using Repatch. Now, once you have that working, it's conceivable that it might be somewhat good to implement *inlining* in addition to *caching*. But your bug is about "caching" rather than "caching and inlining", so I would stick to just doing that one thing. Doing the inlining correctly is going to involve some of the code that you wrote, but probably with some changes. Also, it will definitely require supporting MultiPutByOffset+CustomSetter in the FTL, since MultiPutByOffset is an FTL-only node. So anytime you create a MultiPutByOffset with a variant that the FTL doesn't support, you're creating an embarrassing performance regression.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 225506 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=225506&action=review > > > > I think you need to remove all of your changes from PutByIdStatus and PutByIdVariant, and all of your changes to MultiPutByOffset. Those changes are completely wrong and also unnecessary. > > > > > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:107 > > > - || stubInfo.accessType == access_put_by_id_transition_direct); > > > + || stubInfo.accessType == access_put_by_id_transition_direct > > > + || stubInfo.accessType == access_unset); > > > > This change looks superfluous given that you're guarding calls to this with a check for access_unset. > > > > > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h:50 > > > + Custom > > > > Call it CustomSetter. > > > > > Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h:91 > > > + static PutByIdAccess custom( > > > > call it customSetter > > Righto > > Indentation comments elided as they're just Xcode attempting to auto indent and i'm aware of the current mess :D > > > > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1678 > > > + > > > + if (variant.kind() == PutByIdVariant::Custom) { > > > + filter(node->child1(), structure); > > > + m_state.setFoundConstants(true); > > > + m_state.setHaveStructures(true); > > > + clobberWorld(node->origin.semantic, clobberLimit); > > > + done = true; > > > + break; > > > + } > > > > This makes no sense. What are you trying to do? > > > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1706 > > > + if (variant.kind() == PutByIdVariant::Custom) { > > > + if (value.m_currentKnownStructure.contains(variant.structure())) > > > + newSet.addAll(variant.structure()); > > > + continue; > > > + } > > > > What? > > > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1774 > > > + if (status[0].kind() == PutByIdVariant::Custom) { > > > + m_state.setHaveStructures(true); > > > + m_state.setFoundConstants(true); > > > + clobberWorld(node->origin.semantic, clobberLimit); > > > + break; > > > + } > > > > Again, zero sense. Why is this here. > > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1965 > > > + if (putByIdStatus.numVariants() > 1 || putByIdStatus[0].kind() == PutByIdVariant::Custom) { > > > > Huh? > > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1970 > > > - > > > + CRASH(); > > > > What? > glorified printf debugging :D > > > > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:685 > > > + if (node->multiPutByOffsetData().hasCustom()) > > > + return 0; > > > > Why? > > > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:738 > > > + if (node->multiPutByOffsetData().hasCustom()) > > > + return 0; > > > > Why? > > > > > Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:809 > > > + if (node->multiPutByOffsetData().hasCustom()) > > > + return 0; > > > > Why? > > > > If we're calling a setter arbitrary code may execute so you can't CSE across them. > > Though now that i think about it that's clearly stupid - we just know we can't CSE a setter, and the rest of the node data will ensure we don't try ro reuse values that may be mutated across the call. I'm saying you should remove this code. > > > > Source/JavaScriptCore/dfg/DFGClobberize.h:503 > > > + if (node->multiPutByOffsetData().hasCustom()) { > > > + read(World); > > > + write(World); > > > + } > > > > This is really wrong. > > Why? a custom setter can run arbitrary code so can read/write anything I'm saying you should remove this. > > > > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:192 > > > + if (variant.kind() == PutByIdVariant::Custom) > > > + continue; > > > > OK. This really makes no sense. Why does the abstract interpreter claim to have found a constant when it sees PutByIdVariant::Custom, only so that constant folding can reject PutByIdVariant::Custom? > > *shrug* > > > > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:250 > > > - if (status.numVariants() != 1) > > > + if (status.numVariants() != 1 && status[0].kind() == PutByIdVariant::Custom) > > > > I suspect that you meant || here, but also, this is just wrong. > > This && should be ||, as i was just trying to minimise code i was writing until i had the basics working > > > > > > Source/JavaScriptCore/ftl/FTLCapabilities.cpp:149 > > > + > > > + case MultiPutByOffset: > > > + if (node->multiPutByOffsetData().hasCustom()) > > > + return CannotCompile; > > > + break; > > > + > > > > Why are you creating MultiPutByOffsets with custom setters only to reject them? This means that any code that calls custom setters will no longer compile with the FTL, whereas previous to your change, it would have compiled in the FTL just fine. > > Again, minimising the working surface to just the DFG while i work out what the DFG should be doing. No, you're minimizing the working surface to *zero* because MultiPutByOffset is a FTL-only node.
Created attachment 225515 [details] Updated patch, only works for baseline, dfg crashes
Comment on attachment 225515 [details] Updated patch, only works for baseline, dfg crashes View in context: https://bugs.webkit.org/attachment.cgi?id=225515&action=review > Source/JavaScriptCore/jit/Repatch.cpp:1149 > + allocator.preserveReusedRegistersByPushing(stubJit); Where is the call to restore registers?
Created attachment 225525 [details] Patch
Comment on attachment 225525 [details] Patch dfg still hosed
Attachment 225525 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1171: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1213: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1271: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1274: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1275: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1276: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1319: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1398: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1399: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/Repatch.cpp:1400: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdVariant.h:75: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:199: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/PutByIdStatus.h:65: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 15 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Okay, so the problem i'm hitting is that SpeculativeJIT::cachedPutById is trying to load the structure from the base register, which i've somehow fucked up :-O
Comment on attachment 225525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225525&action=review > Source/JavaScriptCore/jit/Repatch.cpp:1158 > + if (allocator.didReuseRegisters()) > + allocator.restoreReusedRegistersByPopping(stubJit); This is in the wrong place. Here's the thing. You're making a function call. The function you're calling is going to clobber whatever registers it feels like clobbering. The DFG's register allocator assumes that a PutById won't clobber any registers - or at least, that if it does clobber registers, it will restore them. What you're doing here with registers is triply wrong: - You're only saving the registers that you explicitly use, rather than all of the registers that the DFG was expecting to have preserved across the PutById. Note that stubInfo.patch.usedRegisters tells you the set of registers that the DFG is expecting to have preserved. - You're restoring the registers before making the call. Hence, the call will clobber them and you won't restore them after it returns. - You're using ScratchRegisterAllocator, which only knows about how to save and restore "temp registers" - i.e. the registers that our own JITs would use. But, the FTL may use any register on the system. ScratchRegisterAllocator won't know how to save non-temp registers that the FTL might use. ScratchRegisterAllocator is only intended for cases where you want to use one of the temp registers, and you need a thingy to save that temp register if the outer compiler had been using it. You need a different thingy for saving and restoring registers if you intend to do something (like a call) that may clobber some registers other than the temp registers. There are two strategies for dealing with this problem: 1) Do like GetById, and arrange to have the DFG + FTL assume that we didn't preserve any registers. - GetByIdStatus has a MakesCalls in addition to a TakesSlowPath. MakesCalls is what we return when the baseline JIT had done getter caching. - The DFG bytecode parser will emit a GetByIdFlush, rather than a GetById, if the GetByIdStatus said MakesCalls. - GetByIdFlush calls flushRegisters() prior to doing the cachedGetById, and it marks all registers as being flushed in the StructureStubInfo (I forget the name of the bit but it's something obvious like "registersFlushed") - Repatch only caches getters if "registers were flushed". This is always true in baseline, but it's only true in DFG+FTL if we had also cached the getter in baseline. Of course that's a fair assumption. Basically, you could apply this technique by applying s/GetById/PutById/g to my four bullet points, and it would probably work. But, that would still leave the same bug that the GetById stuff has: if a GetById happened to not take the getter caching path in baseline, then the DFG will *never* be able to take the getter caching path. That's dumb. We could fix that in one of two ways: (1) if we try to cache a getter (or setter) in DFG/FTL with !registersFlushed, then set a bit in the StructureStubInfo saying that this thing calls getters and then do codeBlock->jettison() on the DFG/FTL code block so that we will recompile with the knowledge that getters (or setters) are being called here; and/or (2) have Repatch save/restore all registers, as I list below. 2) Create a thingy to save/restore all registers in the usedRegisters set. This is actually surprisingly annoying to write, but it's possible. You could write such a thing, and it would probably have an API that somewhat resembles ScratchRegisterAllocator, except that with this new thingy, you wouldn't ever have to manually lock registers if you wanted to use them - your new thingy would *save all registers* so after it "preserve" function ran, you would be able to use any register you like without worrying about a damn thing. This solution could be used either as an alternative to implementing PutByIdFlush, or it could be done in addition to PutByIdFlush as a mechanism for defending against PutById's that only called setters in the DFG but not baseline. I would strongly recommend implementing (1) for now, and then doing (2) later for both GetById and PutById. I actually think that you could be smart about it and combine (2) with the jettison idea: if a ById access goes to an accessor with !registersFlushed, then: - If the usedRegisters set is small then just save/restore all registers - If the usedRegisters set is large, then jettison, under the assumption that it would be less expensive in the long run to recompile than it would be to keep saving and restoring a ton of registers.
Comment on attachment 225525 [details] Patch This patch wasn't up for review, yet marking r- to get it out of EWS - it causes so much flakiness that EWS keeps trying to figure out what's broken.
Created attachment 225667 [details] Patch
Created attachment 225674 [details] Patch
Comment on attachment 225674 [details] Patch Attachment 225674 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6695957395668992 New failing tests: http/tests/media/reload-after-dialog.html fast/forms/label/labels-remove-htmlFor-attribute.html compositing/video/video-controls-layer-creation.html fast/css/counters/complex-before.html fast/forms/label/labels-parent-and-sibling-labels.html fast/forms/label/labels-change-htmlFor-attribute.html fast/forms/label/labels-remove-htmlFor-label.html fast/canvas/webgl/gl-enable-enum-test.html accessibility/media-controls.html http/tests/media/pdf-served-as-pdf.html compositing/regions/video-in-overflow-region.html fast/forms/label/labels-add-parent-label.html fast/css/relative-position-replaced-in-table-display-crash.html fast/forms/label/labels-remove-parent-label.html accessibility/aria-hidden-hides-all-elements.html fast/forms/label/labels-add-htmlFor-label.html accessibility/media-element.html fast/forms/label/labelable-elements.html http/tests/media/text-served-as-text.html fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html http/tests/media/media-document.html fast/forms/label/labels-set-htmlFor-attribute.html fast/block/child-not-removed-from-parent-lineboxes-crash.html compositing/geometry/clipped-video-controller.html http/tests/media/video-accept-encoding.html fast/canvas/webgl/gl-uniformmatrix4fv.html fast/canvas/webgl/oes-texture-half-float-with-video.html fast/css/first-letter-block-form-controls-crash.html fast/canvas/getPutImageDataPairTest.html fast/forms/label/labels-multiple-sibling-labels.html
Created attachment 225695 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 225702 [details] Patch
Attachment 225702 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 225708 [details] Patch
Attachment 225708 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 225708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225708&action=review So close to done! Needs just a bit of love though. > Source/JavaScriptCore/bytecode/StructureStubInfo.h:197 > - int8_t registersFlushed; > + SpillRegistersMode spillMode : 8; OMG yes! > Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:137 > - : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base, value, registersFlushed) > - , m_scratch(scratch) > - , m_ecmaMode(ecmaMode) > - , m_putKind(putKind) > + : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base, value, spillMode) > + , m_scratch(scratch) > + , m_ecmaMode(ecmaMode) > + , m_putKind(putKind) Pretty sure the original indentation was correct. > Source/JavaScriptCore/jit/Repatch.cpp:1151 > + GPRReg scratchGPR1 = InvalidGPRReg; > + if (prototypeChain) > + scratchGPR1 = allocator.allocateScratchGPR(); > + > + allocator.preserveReusedRegistersByPushing(stubJit); You should get rid of the ScratchRegisterAllocator code and instead assert that there is always a scratch register available directly from the TempRegisterSet, like: TempRegisterSet(stubInfo.patch.usedRegisters).getFreeGPR() And then assert that you get back something other than InvalidGPRReg. You're guaranteed to have a free register because you flushed all registers, and you flushed all registers because you're making a call. > Source/JavaScriptCore/jit/Repatch.cpp:1185 > + if (allocator.didReuseRegisters()) > + allocator.restoreReusedRegistersByPopping(stubJit); This can die now that you're flushing. > Source/JavaScriptCore/jit/Repatch.cpp:1275 > + if (normalizePrototypeChain(exec, baseCell) == InvalidPrototypeChain) Pretty sure you should be calling normalizePrototypeChainForChainAccess().
Created attachment 225794 [details] Not quite right, something weird is happening when writing to globals
Attachment 225794 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 225803 [details] yet more WIP, currently disabled
Attachment 225803 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 225890 [details] Trying to aggressivley disable things while i work out what is going wrong
Attachment 225890 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
I'm seeing weird failures in the _llint_ which essentially should have no behaviour changes so i'm investigating that atm. If anyone could scan the patch and spot any obvious oversights that would be appreciated.
(In reply to comment #31) > I'm seeing weird failures in the _llint_ which essentially should have no behaviour changes so i'm investigating that atm. If anyone could scan the patch and spot any obvious oversights that would be appreciated. turns out drt isn't respecting jsc environment variables?
Created attachment 225895 [details] Patch
Attachment 225895 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:113: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #31) > I'm seeing weird failures in the _llint_ which essentially should have no behaviour changes so i'm investigating that atm. If anyone could scan the patch and spot any obvious oversights that would be appreciated. What do you mean by _llint_?
Comment on attachment 225895 [details] Patch Attachment 225895 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5266935071637504 New failing tests: css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html editing/inserting/insert-composition-whitespace.html css3/compositing/background-blend-mode-property-parsing.html accessibility/canvas-fallback-content-2.html css3/flexbox/css-properties.html css3/unicode-bidi-isolate-basic.html accessibility/canvas-fallback-content.html editing/selection/move-by-character-brute-force.html css3/flexbox/flex-property-parsing.html fast/canvas/canvas-blend-solid.html fast/canvas/canvas-blending-color-over-gradient.html css3/filters/filter-property-parsing.html fast/css-grid-layout/grid-columns-rows-get-set.html css3/compositing/blend-mode-property-parsing.html editing/execCommand/enabling-and-selection-2.html editing/selection/programmatic-selection-on-mac-is-directionless.html fast/canvas/canvas-blending-color-over-color.html css3/filters/filter-property-computed-style.html editing/execCommand/query-format-block.html fast/canvas/canvas-blending-clipping.html css3/filters/filter-property-parsing-invalid.html editing/selection/block-cursor-overtype-mode.html http/tests/css/shared-stylesheet-mutation-preconstruct.html editing/selection/select-bidi-run.html css3/zoom-coords.xhtml fast/canvas/canvas-blend-image.html fast/css-grid-layout/grid-columns-rows-get-set-multiple.html fast/backgrounds/background-shorthand-with-backgroundSize-style.html fast/backgrounds/background-position-parsing-2.html editing/execCommand/query-command-state.html
Created attachment 225901 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 225895 [details] Patch Attachment 225895 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4946754923397120 New failing tests: css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html editing/inserting/insert-composition-whitespace.html css3/compositing/background-blend-mode-property-parsing.html accessibility/canvas-fallback-content-2.html css3/flexbox/css-properties.html fast/css-grid-layout/grid-item-end-after-get-set.html css3/unicode-bidi-isolate-basic.html accessibility/canvas-fallback-content.html editing/selection/move-by-character-brute-force.html css3/flexbox/flex-property-parsing.html fast/canvas/canvas-blend-solid.html fast/canvas/canvas-blending-color-over-gradient.html css3/filters/filter-property-parsing.html fast/css-grid-layout/grid-columns-rows-get-set.html css3/compositing/blend-mode-property-parsing.html editing/execCommand/enabling-and-selection-2.html fast/canvas/canvas-blending-color-over-color.html css3/filters/filter-property-computed-style.html editing/execCommand/query-format-block.html fast/canvas/canvas-blending-clipping.html css3/filters/filter-property-parsing-invalid.html editing/selection/block-cursor-overtype-mode.html http/tests/css/shared-stylesheet-mutation-preconstruct.html css3/zoom-coords.xhtml fast/canvas/canvas-blend-image.html fast/css-grid-layout/grid-columns-rows-get-set-multiple.html fast/backgrounds/background-shorthand-with-backgroundSize-style.html fast/backgrounds/background-position-parsing-2.html fast/css-grid-layout/grid-item-column-row-get-set.html editing/execCommand/query-command-state.html
Created attachment 225906 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 225895 [details] Patch Attachment 225895 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5925036166742016 New failing tests: css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html editing/inserting/insert-composition-whitespace.html css3/compositing/background-blend-mode-property-parsing.html accessibility/canvas-fallback-content-2.html css3/flexbox/css-properties.html fast/css-grid-layout/grid-item-end-after-get-set.html css3/unicode-bidi-isolate-basic.html accessibility/canvas-fallback-content.html editing/selection/move-by-character-brute-force.html css3/flexbox/flex-property-parsing.html fast/canvas/canvas-blend-solid.html fast/canvas/canvas-blending-color-over-gradient.html css3/filters/filter-property-parsing.html fast/css-grid-layout/grid-columns-rows-get-set.html css3/compositing/blend-mode-property-parsing.html editing/execCommand/enabling-and-selection-2.html fast/canvas/canvas-blending-color-over-color.html css3/filters/filter-property-computed-style.html editing/execCommand/query-format-block.html fast/canvas/canvas-blending-clipping.html css3/filters/filter-property-parsing-invalid.html editing/selection/block-cursor-overtype-mode.html http/tests/css/shared-stylesheet-mutation-preconstruct.html css3/zoom-coords.xhtml fast/canvas/canvas-blend-image.html fast/css-grid-layout/grid-columns-rows-get-set-multiple.html fast/backgrounds/background-shorthand-with-backgroundSize-style.html fast/backgrounds/background-position-parsing-2.html fast/css-grid-layout/grid-item-column-row-get-set.html editing/execCommand/query-command-state.html
Created attachment 225910 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 225895 [details] Patch I think you need to investigate why all of those tests are failing.
(In reply to comment #42) > (From update of attachment 225895 [details]) > I think you need to investigate why all of those tests are failing. Yup, still investigating
Created attachment 225916 [details] Patch
Attachment 225916 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 225916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225916&action=review Fix indentation. Or, rather, bring it back to the way it was. Note that the style bot is totally busted with respect to constructor indentation. > Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:137 > - : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base, value, registersFlushed) > - , m_scratch(scratch) > - , m_ecmaMode(ecmaMode) > - , m_putKind(putKind) > + : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base, value, spillMode) > + , m_scratch(scratch) > + , m_ecmaMode(ecmaMode) > + , m_putKind(putKind) The original indentation was correct. > Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:100 > + : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base, value, spillMode) The original indentation was correct. > Source/JavaScriptCore/jit/Repatch.cpp:1150 > + GPRReg scratchGPR1 = InvalidGPRReg; > + if (prototypeChain) { > + scratchGPR1 = tempRegisters.getFreeGPR(); > + RELEASE_ASSERT(scratchGPR1 != InvalidGPRReg); > + } Why not just always call tempRegisters.getFreeGPR(), and always assert that you got one? No point in predicating this on prototypeChain.
Committed r165141: <http://trac.webkit.org/changeset/165141>
I couldn't see any progression on DYEBench but this was a 5% progression on Dromaeo DOM Core. In particular, dom-attr improved by 20% (measured on my MacPro): http://dromaeo.com/?id=218087,218088,218092 Now we're ahead of Firefox by 9%.
FTL build fixes: http://trac.webkit.org/changeset/165157 http://trac.webkit.org/changeset/165158
Re-opened since this is blocked by bug 129769
The build has been fixed.
Re-opened since this is blocked by bug 129772
This was also ~1.5% win on CSSSelector tests: http://dromaeo.com/?id=218103,218104
Committed r165208: <http://trac.webkit.org/changeset/165208>
Comment on attachment 225803 [details] yet more WIP, currently disabled Cleared review? from obsolete attachment 225803 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).