Bug 201373 - [JSC] Merge op_check_traps into op_enter and op_loop_hint
Summary: [JSC] Merge op_check_traps into op_enter and op_loop_hint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-30 20:02 PDT by Yusuke Suzuki
Modified: 2019-09-04 22:52 PDT (History)
7 users (show)

See Also:


Attachments
Patch (22.43 KB, patch)
2019-08-30 20:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (23.72 KB, patch)
2019-08-30 20:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (4.95 MB, application/zip)
2019-08-30 22:02 PDT, Build Bot
no flags Details
Patch (26.96 KB, patch)
2019-08-31 04:21 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.32 KB, patch)
2019-08-31 05:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (3.79 MB, application/zip)
2019-08-31 07:26 PDT, Build Bot
no flags Details
Patch (27.28 KB, patch)
2019-08-31 23:25 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2019-09-01 02:05 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2019-09-01 03:35 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff
Patch for landing (28.27 KB, patch)
2019-09-01 16:30 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 2019-08-30 20:02:11 PDT
[JSC] Merge op_check_traps into op_enter and op_loop_hint
Comment 1 Yusuke Suzuki 2019-08-30 20:19:25 PDT
Created attachment 377782 [details]
Patch
Comment 2 Yusuke Suzuki 2019-08-30 20:32:42 PDT
Created attachment 377785 [details]
Patch
Comment 3 Mark Lam 2019-08-30 21:18:51 PDT
Comment on attachment 377785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377785&action=review

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1034
> +#if ENABLE(DFG_JIT)
> +    // Emit the JIT optimization check:
>      if (canBeOptimized()) {
>          addSlowCase(branchAdd32(PositiveOrZero, TrustedImm32(Options::executionCounterIncrementForLoop()),
>              AbsoluteAddress(m_codeBlock->addressOfJITExecuteCounter())));
>      }
> +#endif
> +    // Check traps.
> +    addSlowCase(branchTest8(NonZero, AbsoluteAddress(m_vm->needTrapHandlingAddress())));

I think we want to check traps before we check for optimization.  This is because if we OSR exit due to a trap, we want to handle the trap first.  Otherwise, there's a chance we'll be caught in a optimize + OSR exit loop.  In practice, this might not happen, but I think it is more correct to check traps first.

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1019
> +    emitEnterOptimizationCheck();
> +
> +    // Check traps.
> +    addSlowCase(branchTest8(NonZero, AbsoluteAddress(m_vm->needTrapHandlingAddress())));

Ditto here.  Let's check traps first before checking for optimizing.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1678
>      checkSwitchToJITForLoop()
> -    dispatch()
> -end)
> -
> -
> -llintOp(op_check_traps, OpCheckTraps, macro (unused, unused, dispatch)
> +    # CheckTraps.
>      loadp CodeBlock[cfr], t1
>      loadp CodeBlock::m_vm[t1], t1
> -    loadb VM::m_traps+VMTraps::m_needTrapHandling[t1], t0
> -    btpnz t0, .handleTraps
> +    btpnz VM::m_traps + VMTraps::m_needTrapHandling[t1], .handleTraps

Ditto here.  Let's check traps first.  This will be meaningful if we do OSR exit to LLInt later.
Comment 4 Build Bot 2019-08-30 22:02:08 PDT
Comment on attachment 377785 [details]
Patch

Attachment 377785 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12986566

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2019-08-30 22:02:09 PDT
Created attachment 377786 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 6 Build Bot 2019-08-30 22:54:12 PDT
Comment on attachment 377785 [details]
Patch

Attachment 377785 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12986568

New failing tests:
stress/regress-189227-watchdog-on-infinite-loop.js.dfg-eager-no-cjit-validate
stress/call-link-info-osrexit-repatch.js.ftl-eager
stress/regress-189227-watchdog-on-infinite-loop.js.eager-jettison-no-cjit
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-validate-sampling-profiler
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-eager-no-cjit
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-b3o0
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-eager-no-cjit-b3o1
stress/regress-189227-watchdog-on-infinite-loop.js.no-cjit-collect-continuously
stress/regress-189227-watchdog-on-infinite-loop.js.no-cjit-validate-phases
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-no-inline-validate
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-no-put-stack-validate
Comment 7 Yusuke Suzuki 2019-08-31 04:14:30 PDT
Comment on attachment 377785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377785&action=review

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:1034
>> +    addSlowCase(branchTest8(NonZero, AbsoluteAddress(m_vm->needTrapHandlingAddress())));
> 
> I think we want to check traps before we check for optimization.  This is because if we OSR exit due to a trap, we want to handle the trap first.  Otherwise, there's a chance we'll be caught in a optimize + OSR exit loop.  In practice, this might not happen, but I think it is more correct to check traps first.

OK, fixed.

>> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1019
>> +    addSlowCase(branchTest8(NonZero, AbsoluteAddress(m_vm->needTrapHandlingAddress())));
> 
> Ditto here.  Let's check traps first before checking for optimizing.

Fixed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1678
>> +    btpnz VM::m_traps + VMTraps::m_needTrapHandling[t1], .handleTraps
> 
> Ditto here.  Let's check traps first.  This will be meaningful if we do OSR exit to LLInt later.

Fixed.
Comment 8 Yusuke Suzuki 2019-08-31 04:21:25 PDT
Created attachment 377798 [details]
Patch
Comment 9 Yusuke Suzuki 2019-08-31 05:32:29 PDT
Created attachment 377799 [details]
Patch
Comment 10 Build Bot 2019-08-31 07:26:26 PDT
Comment on attachment 377799 [details]
Patch

Attachment 377799 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12987339

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2019-08-31 07:26:28 PDT
Created attachment 377801 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 12 Build Bot 2019-08-31 07:51:54 PDT
Comment on attachment 377799 [details]
Patch

Attachment 377799 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12987334

New failing tests:
stress/call-link-info-osrexit-repatch.js.ftl-eager
Comment 13 Yusuke Suzuki 2019-08-31 23:25:42 PDT
Created attachment 377809 [details]
Patch
Comment 14 Build Bot 2019-09-01 01:44:37 PDT
Comment on attachment 377809 [details]
Patch

Attachment 377809 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12988577

New failing tests:
stress/call-link-info-osrexit-repatch.js.ftl-eager
Comment 15 Yusuke Suzuki 2019-09-01 02:05:38 PDT
Created attachment 377812 [details]
Patch
Comment 16 Yusuke Suzuki 2019-09-01 03:35:30 PDT
Created attachment 377816 [details]
Patch
Comment 17 Mark Lam 2019-09-01 06:34:17 PDT
Comment on attachment 377816 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377816&action=review

Nice work.  r=me

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:727
> +    callTrapHandler(.throwHandler)

Why not also callTrapHandler(_llint_throw_from_slow_path_trampoline) here like you did in llintOp(op_loop_hint)?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:730
> +.throwHandler:
> +    jmp _llint_throw_from_slow_path_trampoline

Not needed if you callTrapHandler(_llint_throw_from_slow_path_trampoline) above.
Comment 18 Yusuke Suzuki 2019-09-01 16:29:17 PDT
Comment on attachment 377816 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377816&action=review

Thanks!

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:727
>> +    callTrapHandler(.throwHandler)
> 
> Why not also callTrapHandler(_llint_throw_from_slow_path_trampoline) here like you did in llintOp(op_loop_hint)?

Right. Fixed.
Comment 19 Yusuke Suzuki 2019-09-01 16:30:15 PDT
Created attachment 377830 [details]
Patch for landing
Comment 20 Yusuke Suzuki 2019-09-01 20:44:37 PDT
Committed r249372: <https://trac.webkit.org/changeset/249372>
Comment 21 Radar WebKit Bug Importer 2019-09-01 20:46:24 PDT
<rdar://problem/54939747>
Comment 22 Yusuke Suzuki 2019-09-04 22:41:39 PDT
I'll roll-out this patch for now since JetStream2/Basic shows ~10% regression.
My guess is that this regression is not inherently related to this patch: changing size of bytecodes and hitting some spot. But I'm not sure, I'll revisit this after generator optimization is done.
Comment 23 Yusuke Suzuki 2019-09-04 22:52:01 PDT
Committed r249523: <https://trac.webkit.org/changeset/249523>