Change baseline JIT watchdog timer check to use proper fast slow path infrastructure

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

The implementation of the watchdog timer check in the base JIT did not use the proper fast and slow path infrastructure that was already in place.  This patch refactors the code to use that infrastructure and be consistent with how other opcodes handle such fast / slow paths.

[Mark Lam]

This patch till break the SH4 port because I can’t find how it expresses the PositiveOrZero condition code.  I’ll have to leave it to the port to fix the issue.

[Julien Brianceau]

(In reply to comment #1)
> This patch till break the SH4 port because I can’t find how it expresses the PositiveOrZero condition code.  I’ll have to leave it to the port to fix the issue.
Can you share with me your what's missing in the SH4 port for your patch? I'm available in #webkit channel.
Thanks !

[Mark Lam]

Created attachment 199042 [details]
the patch.

Perf-wise, the patch seems to be a wash on x86 (benchmark results will be attached shortly).  Regardless, I think it’s the right thing to have the code be consistent with how all the other baseline JIT opcodes handle fast slow paths.

[Mark Lam]

Created attachment 199043 [details]
benchmark result: DFG disabled using useDFGJIT()=false, watchdog disabled.

[Mark Lam]

Created attachment 199044 [details]
benchmark result: run 2: DFG disabled using useDFGJIT()=false, watchdog disabled.

[WebKit Commit Bot]

Attachment 199042 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/ARMAssembler.h', u'Source/JavaScriptCore/assembler/MacroAssemblerARM.h', u'Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h', u'Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h', u'Source/JavaScriptCore/assembler/MacroAssemblerSH4.h', u'Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h', u'Source/JavaScriptCore/assembler/SH4Assembler.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp']" exit_code: 1
Source/JavaScriptCore/assembler/ARMAssembler.h:124:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 199045 [details]
benchmark result: DFG enabled, watchdog disabled.

[Mark Lam]

Created attachment 199046 [details]
benchmark result: run 2: DFG enabled, watchdog disabled.

[Mark Lam]

FYI, there are some “definitely slower” results in some of the benchmark tests (the ones with the DFG disabled).  However, if you look at the next run, the same test is no longer “definitely slower”.  I’m choosing to ignore this since the overall results are not “definitely slower”.

[Oliver Hunt]

Comment on attachment 199042 [details]
the patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r148893: <http://trac.webkit.org/changeset/148893>.

[Julien Brianceau]

(In reply to comment #11)
> Thanks for the review.  Landed in r148893: <http://trac.webkit.org/changeset/148893>.

FYI SH4 build is broken. I entered a bug https://bugs.webkit.org/show_bug.cgi?id=114968 and I'll provide a fix this evening or tomorrow.

[Geoffrey Garen]

Comment on attachment 199042 [details]
the patch.

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

> Source/JavaScriptCore/jit/JITOpcodes.cpp:493
> +        addSlowCase(branchAdd32(PositiveOrZero, TrustedImm32(Options::executionCounterIncrementForLoop()),
> +            AbsoluteAddress(m_codeBlock->addressOfJITExecuteCounter())));

Why this change to using PositiveOrZero? What does that have to do with the watchdog? This is the kind of thing you should explain in your ChangeLog.

[Geoffrey Garen]

Does this patch change performance when the watchdog is enabled?

[Geoffrey Garen]

Comment on attachment 199042 [details]
the patch.

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

> Source/JavaScriptCore/jit/JITOpcodes.cpp:513
> +#if ENABLE(DFG_JIT)
> +        if (canBeOptimized()) {
> +            Jump doOptimize = branchAdd32(PositiveOrZero, TrustedImm32(Options::executionCounterIncrementForLoop()),
> +                AbsoluteAddress(m_codeBlock->addressOfJITExecuteCounter()));
> +            emitJumpSlowToHot(jump(), OPCODE_LENGTH(op_loop_hint));
> +            doOptimize.link(this);
> +        } else
> +#endif
> +            emitJumpSlowToHot(jump(), OPCODE_LENGTH(op_loop_hint));

It seems like we could save some memory by just skipping out on the counter in the case where we've timed out, or vice versa.

[Mark Lam]

Created attachment 199125 [details]
benchmark result: DFG disabled using useDFGJIT()=false, watchdog enabled.

[Mark Lam]

Created attachment 199126 [details]
benchmark result: run 2: DFG disabled using useDFGJIT()=false, watchdog enabled.

[Mark Lam]

(In reply to comment #13)
> Why this change to using PositiveOrZero? What does that have to do with the watchdog? This is the kind of thing you should explain in your ChangeLog.

As explained in person, the previous test on Signed (aka Negative) was to branch around the slow path.  Since we now need to test for the condition to branch to the slow path, we need to test for the complement of the previous condition i.e. we need to test for PositiveOrZero.

Perf numbers for the baseline JIT (DFG disabled) with the watchdog enabled (but not firing) is attached.  Generally, it's positive with "maybe faster".  Comparing the 2 runs, we see that some "definitely slower"s are not true in the other run.  This is except for some components of Kraken which has some consistent "definitely slower"s, but overall, Kraken is a wash too.

I'll look into the elimination of the slow path code that handles both a watchdog timer check and an optimization check in a subsequent patch.