IC can use DOMJIT::Patchpoint to emit the fast path instead of calling CustomAccessor getter.
*** This bug has been marked as a duplicate of bug 163226 ***
I think this should be done separately from IC's special path.
Created attachment 291465 [details] Patch WIP
Created attachment 291468 [details] Patch WIP
Created attachment 291514 [details] Patch WIP
don-traverse results. Baseline firstChild:Runs -> [976, 979, 1028, 1061, 1066] runs/s mean: 1022 runs/s median: 1028 runs/s stdev: 43.17985641476821 runs/s min: 976 runs/s max: 1066 runs/s lastChild:Runs -> [907, 928, 936, 937, 970] runs/s mean: 935.6 runs/s median: 936 runs/s stdev: 22.700220263248546 runs/s min: 907 runs/s max: 970 runs/s nextSibling:Runs -> [1345, 1379, 1418, 1437, 1448] runs/s mean: 1405.4 runs/s median: 1418 runs/s stdev: 42.77031680967534 runs/s min: 1345 runs/s max: 1448 runs/s previousSibling:Runs -> [1281, 1336, 1354, 1356, 1370] runs/s mean: 1339.4 runs/s median: 1354 runs/s stdev: 34.81091782760112 runs/s min: 1281 runs/s max: 1370 runs/s childNodes:Runs -> [1502, 1508, 1509, 1525, 1535] runs/s mean: 1515.8 runs/s median: 1509 runs/s stdev: 13.700364958642547 runs/s min: 1502 runs/s max: 1535 runs/s :Runs -> [231.6402243970682, 236.08108445733586, 241.10327162754916, 243.97158510784413, 247.46260021309118] runs/s mean: 240.0517531605777 runs/s median: 241.10327162754916 runs/s stdev: 6.283141206975851 runs/s min: 231.6402243970682 runs/s max: 247.46260021309118 runs/s Patched: firstChild:Runs -> [1166, 1171, 1204, 1219, 1231] runs/s mean: 1198.2 runs/s median: 1204 runs/s stdev: 28.804513535208343 runs/s min: 1166 runs/s max: 1231 runs/s lastChild:Runs -> [1081, 1081, 1108, 1109, 1125] runs/s mean: 1100.8 runs/s median: 1108 runs/s stdev: 19.292485583770702 runs/s min: 1081 runs/s max: 1125 runs/s nextSibling:Runs -> [1388, 1414, 1465, 1515, 1527] runs/s mean: 1461.8 runs/s median: 1465 runs/s stdev: 60.874460983239956 runs/s min: 1388 runs/s max: 1527 runs/s previousSibling:Runs -> [1476, 1480, 1487, 1491, 1516] runs/s mean: 1490 runs/s median: 1487 runs/s stdev: 15.668439615992398 runs/s min: 1476 runs/s max: 1516 runs/s childNodes:Runs -> [1539, 1545, 1546, 1553, 1554] runs/s mean: 1547.4 runs/s median: 1546 runs/s stdev: 6.188699378706323 runs/s min: 1539 runs/s max: 1554 runs/s :Runs -> [261.06642543785387, 262.5237761338508, 267.73681405310816, 270.50936975529527, 273.280292306101] runs/s mean: 267.02333553724185 runs/s median: 267.73681405310816 runs/s stdev: 5.185118294847495 runs/s min: 261.06642543785387 runs/s max: 273.280292306101 runs/s
Created attachment 291551 [details] Patch WIP
Created attachment 291552 [details] Patch WIP
Created attachment 291567 [details] Patch
OK, patch is ready!
Comment on attachment 291567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291567&action=review > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1049 > + I'll extract this part to a function soon.
Created attachment 291574 [details] Patch
Comment on attachment 291574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291574&action=review r=me with comments > Source/JavaScriptCore/bytecode/DOMJITAccessCasePatchpointParams.cpp:83 > + bool isGetter = true; Maybe we should just name this something like "callHasReturnValue" and also the parameter to the function. I think I originally renamed this "isGetter" but I think that's a confusing name given the purpose of the boolean. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:80 > + if (m_extraLiveRegistersForCall != extra) > + m_calculatedRegistersForCallAndExceptionHandling = false; As we spoke about in person, lets migrate this function to something that returns a token that represents the state. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1449 > + if (paramGlobalObjectGPR != InvalidGPRReg) > + regs.append(paramGlobalObjectGPR); Is the API to always pass the GlobalObject as the second parameter if requested by the patchpoint? Nit: Also, I think the code is cleaner as: if (patchpoint->requireGlobalObject) { ASSERT(paramGlobalObjectGPR != InvalidGPRReg); regs.append(paramGlobalObjectGPR); } In case anybody changes the above code, the assertion prevents them from messing up. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1482 > + jit.emitLoadStructure(paramBaseGPR, paramGlobalObjectGPR, valueRegs.payloadGPR()); Will this ever not be the GlobalObject that belongs to the structure we're emitting code for? Seems like we could just move a constant pointer instead of doing a dependent load. > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1487 > + // We do not need to spill locked registers since the caller (Baseline, DFG, and FTL) > + // must assume that they can be clobbered in the IC fast path if it succeeds. As we discussed in person, this comment can be somewhat confusing. I would just say that for things to not be spilled, they must be in the used register set. And some things are allowed to be locked but not in the used register set. (Or some comment along those lines.) > JSTests/stress/domjit-getter-complex-with-incorrect-object.js:1 > +function shouldThrow(func, errorMessage) { Can you also add a test that tiers up, then throws an exception, to make sure we're OSR exitting properly. The test can make sure values are what you expect inside the catch block.
Comment on attachment 291574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291574&action=review Thanks! >> Source/JavaScriptCore/bytecode/DOMJITAccessCasePatchpointParams.cpp:83 >> + bool isGetter = true; > > Maybe we should just name this something like "callHasReturnValue" and also the parameter to the function. I think I originally renamed this "isGetter" but I think that's a confusing name given the purpose of the boolean. Right. We should do that. >> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:80 >> + m_calculatedRegistersForCallAndExceptionHandling = false; > > As we spoke about in person, lets migrate this function to something that returns a token that represents the state. Changed. These functions return SpillState. >> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1449 >> + regs.append(paramGlobalObjectGPR); > > Is the API to always pass the GlobalObject as the second parameter if requested by the patchpoint? > > Nit: Also, I think the code is cleaner as: > if (patchpoint->requireGlobalObject) { > ASSERT(paramGlobalObjectGPR != InvalidGPRReg); > regs.append(paramGlobalObjectGPR); > } > > In case anybody changes the above code, the assertion prevents them from messing up. Yes. We pass it as a second parameter. And this is why I create the specialized patchpoint "CallDOMPatchpoint" from the general "Patchpoint". "CallDOMPatchpoint" is the derived class of "Patchpoint". And only "CallDOMPatchpoint" has this option. And yeah, the code cleanup suggestion looks nice. Fixed. >> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1482 >> + jit.emitLoadStructure(paramBaseGPR, paramGlobalObjectGPR, valueRegs.payloadGPR()); > > Will this ever not be the GlobalObject that belongs to the structure we're emitting code for? Seems like we could just move a constant pointer instead of doing a dependent load. Yes, right! We don't need to load a global object here! >> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1487 >> + // must assume that they can be clobbered in the IC fast path if it succeeds. > > As we discussed in person, this comment can be somewhat confusing. I would just say that for things to not be spilled, they must be in the used register set. And some things are allowed to be locked but not in the used register set. (Or some comment along those lines.) Fix this comment. >> JSTests/stress/domjit-getter-complex-with-incorrect-object.js:1 >> +function shouldThrow(func, errorMessage) { > > Can you also add a test that tiers up, then throws an exception, to make sure we're OSR exitting properly. The test can make sure values are what you expect inside the catch block. Sure! Added.
Created attachment 291740 [details] Patch for landing
Comment on attachment 291574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291574&action=review >>> JSTests/stress/domjit-getter-complex-with-incorrect-object.js:1 >>> +function shouldThrow(func, errorMessage) { >> >> Can you also add a test that tiers up, then throws an exception, to make sure we're OSR exitting properly. The test can make sure values are what you expect inside the catch block. > > Sure! Added. After cleaning up the patch, I've found one bug: forgetting resuming registers & stacks when exception occurs inside the slow path. I've fixed in that clean up and added the test to ensure the fix.
Created attachment 291861 [details] Patch for landing
Committed r207427: <http://trac.webkit.org/changeset/207427>
Committed r207428: <http://trac.webkit.org/changeset/207428>
(In reply to comment #19) > Committed r207428: <http://trac.webkit.org/changeset/207428> It broke the Apple Mac build, see build.webkit.org for details.
Committed r207430: <http://trac.webkit.org/changeset/207430>
(In reply to comment #20) > (In reply to comment #19) > > Committed r207428: <http://trac.webkit.org/changeset/207428> > > It broke the Apple Mac build, see build.webkit.org for details. Thanks, I've just fixed.
And this issue figures out we need to clean up IC code. https://bugs.webkit.org/show_bug.cgi?id=163563
** The following JSC stress test failures have been introduced: stress/domjit-exception.js.default stress/domjit-exception.js.dfg-eager stress/domjit-exception.js.dfg-maximal-flush-validate-no-cjit stress/domjit-exception.js.ftl-eager stress/domjit-exception.js.ftl-eager-no-cjit stress/domjit-exception.js.ftl-no-cjit-no-inline-validate stress/domjit-exception.js.ftl-no-cjit-no-put-stack-validate stress/domjit-exception.js.ftl-no-cjit-small-pool stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler stress/domjit-exception.js.no-cjit-validate-phases stress/domjit-exception.js.no-ftl stress/domjit-exception.js.no-llint https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/4319
(In reply to comment #24) > ** The following JSC stress test failures have been introduced: > stress/domjit-exception.js.default > stress/domjit-exception.js.dfg-eager > stress/domjit-exception.js.dfg-maximal-flush-validate-no-cjit > stress/domjit-exception.js.ftl-eager > stress/domjit-exception.js.ftl-eager-no-cjit > stress/domjit-exception.js.ftl-no-cjit-no-inline-validate > stress/domjit-exception.js.ftl-no-cjit-no-put-stack-validate > stress/domjit-exception.js.ftl-no-cjit-small-pool > stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler > stress/domjit-exception.js.no-cjit-validate-phases > stress/domjit-exception.js.no-ftl > stress/domjit-exception.js.no-llint > > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/4319 Thanks! Looking.
(In reply to comment #25) > (In reply to comment #24) > > ** The following JSC stress test failures have been introduced: > > stress/domjit-exception.js.default > > stress/domjit-exception.js.dfg-eager > > stress/domjit-exception.js.dfg-maximal-flush-validate-no-cjit > > stress/domjit-exception.js.ftl-eager > > stress/domjit-exception.js.ftl-eager-no-cjit > > stress/domjit-exception.js.ftl-no-cjit-no-inline-validate > > stress/domjit-exception.js.ftl-no-cjit-no-put-stack-validate > > stress/domjit-exception.js.ftl-no-cjit-small-pool > > stress/domjit-exception.js.ftl-no-cjit-validate-sampling-profiler > > stress/domjit-exception.js.no-cjit-validate-phases > > stress/domjit-exception.js.no-ftl > > stress/domjit-exception.js.no-llint > > > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/4319 > > Thanks! Looking. Ah, OK. This is because we don't set NativeCallFrameTracer. Slow call should use these things since they are CCalls. I'll create the patch to fix it...!