Bug 201373 – [JSC] Merge op_check_traps into op_enter and op_loop_hint

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Merge op_check_traps into op_enter and op_loop_hint

[Yusuke Suzuki]

Created attachment 377782 [details]
Patch

[Yusuke Suzuki]

Created attachment 377785 [details]
Patch

[Mark Lam]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 377798 [details]
Patch

[Yusuke Suzuki]

Created attachment 377799 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 377809 [details]
Patch

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 377812 [details]
Patch

[Yusuke Suzuki]

Created attachment 377816 [details]
Patch

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 377830 [details]
Patch for landing

[Yusuke Suzuki]

Committed r249372: <https://trac.webkit.org/changeset/249372>

[Radar WebKit Bug Importer]

<rdar://problem/54939747>

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r249523: <https://trac.webkit.org/changeset/249523>