Bug 163223 - [DOMJIT] Use DOMJIT::Patchpoint in IC
Summary: [DOMJIT] Use DOMJIT::Patchpoint in IC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 163005
Blocks: 162544 163226 163563 163457 163586
  Show dependency treegraph
 
Reported: 2016-10-10 10:53 PDT by Yusuke Suzuki
Modified: 2016-10-17 23:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.55 KB, patch)
2016-10-13 01:12 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (16.94 KB, patch)
2016-10-13 01:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (32.22 KB, patch)
2016-10-13 14:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (44.11 KB, patch)
2016-10-13 18:15 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (44.13 KB, patch)
2016-10-13 18:23 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (46.37 KB, patch)
2016-10-13 22:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (46.24 KB, patch)
2016-10-13 23:17 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (66.02 KB, patch)
2016-10-16 02:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch for landing (68.78 KB, patch)
2016-10-17 13:26 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-10-10 10:53:35 PDT
IC can use DOMJIT::Patchpoint to emit the fast path instead of calling CustomAccessor getter.
Comment 1 Yusuke Suzuki 2016-10-11 22:34:44 PDT

*** This bug has been marked as a duplicate of bug 163226 ***
Comment 2 Yusuke Suzuki 2016-10-12 23:05:49 PDT
I think this should be done separately from IC's special path.
Comment 3 Yusuke Suzuki 2016-10-13 01:12:47 PDT
Created attachment 291465 [details]
Patch

WIP
Comment 4 Yusuke Suzuki 2016-10-13 01:48:36 PDT
Created attachment 291468 [details]
Patch

WIP
Comment 5 Yusuke Suzuki 2016-10-13 14:08:27 PDT
Created attachment 291514 [details]
Patch

WIP
Comment 6 Yusuke Suzuki 2016-10-13 17:56:17 PDT
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
Comment 7 Yusuke Suzuki 2016-10-13 18:15:03 PDT
Created attachment 291551 [details]
Patch

WIP
Comment 8 Yusuke Suzuki 2016-10-13 18:23:50 PDT
Created attachment 291552 [details]
Patch

WIP
Comment 9 Yusuke Suzuki 2016-10-13 22:14:38 PDT
Created attachment 291567 [details]
Patch
Comment 10 Yusuke Suzuki 2016-10-13 22:14:58 PDT
OK, patch is ready!
Comment 11 Yusuke Suzuki 2016-10-13 23:07:50 PDT
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.
Comment 12 Yusuke Suzuki 2016-10-13 23:17:17 PDT
Created attachment 291574 [details]
Patch
Comment 13 Saam Barati 2016-10-14 14:46:00 PDT
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 14 Yusuke Suzuki 2016-10-16 00:31:34 PDT
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.
Comment 15 Yusuke Suzuki 2016-10-16 02:13:22 PDT
Created attachment 291740 [details]
Patch for landing
Comment 16 Yusuke Suzuki 2016-10-17 13:08:25 PDT
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.
Comment 17 Yusuke Suzuki 2016-10-17 13:26:33 PDT
Created attachment 291861 [details]
Patch for landing
Comment 18 Yusuke Suzuki 2016-10-17 13:46:27 PDT
Committed r207427: <http://trac.webkit.org/changeset/207427>
Comment 19 Yusuke Suzuki 2016-10-17 14:00:12 PDT
Committed r207428: <http://trac.webkit.org/changeset/207428>
Comment 20 Csaba Osztrogonác 2016-10-17 14:13:01 PDT
(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.
Comment 21 Yusuke Suzuki 2016-10-17 14:13:17 PDT
Committed r207430: <http://trac.webkit.org/changeset/207430>
Comment 22 Yusuke Suzuki 2016-10-17 14:26:00 PDT
(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.
Comment 23 Yusuke Suzuki 2016-10-17 15:28:06 PDT
And this issue figures out we need to clean up IC code.
https://bugs.webkit.org/show_bug.cgi?id=163563
Comment 24 Ryan Haddad 2016-10-17 22:49:08 PDT
** 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
Comment 25 Yusuke Suzuki 2016-10-17 23:01:45 PDT
(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.
Comment 26 Yusuke Suzuki 2016-10-17 23:23:44 PDT
(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...!