Bug 163223 – [DOMJIT] Use DOMJIT::Patchpoint in IC

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

[Yusuke Suzuki]

IC can use DOMJIT::Patchpoint to emit the fast path instead of calling CustomAccessor getter.

[Yusuke Suzuki]

*** This bug has been marked as a duplicate of bug 163226 ***

[Yusuke Suzuki]

I think this should be done separately from IC's special path.

[Yusuke Suzuki]

Created attachment 291465 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 291468 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 291514 [details]
Patch

WIP

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 291551 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 291552 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 291567 [details]
Patch

[Yusuke Suzuki]

OK, patch is ready!

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 291574 [details]
Patch

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 291740 [details]
Patch for landing

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 291861 [details]
Patch for landing

[Yusuke Suzuki]

Committed r207427: <http://trac.webkit.org/changeset/207427>

[Yusuke Suzuki]

Committed r207428: <http://trac.webkit.org/changeset/207428>

[Csaba Osztrogonác]

(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.

[Yusuke Suzuki]

Committed r207430: <http://trac.webkit.org/changeset/207430>

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

And this issue figures out we need to clean up IC code.
https://bugs.webkit.org/show_bug.cgi?id=163563

[Ryan Haddad]

** 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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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...!