Bug 114963 - Change baseline JIT watchdog timer check to use proper fast slow path infrastructure
Summary: Change baseline JIT watchdog timer check to use proper fast slow path infrast...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 116538
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-22 09:36 PDT by Mark Lam
Modified: 2013-05-21 08:19 PDT (History)
7 users (show)

See Also:


Attachments
the patch. (14.64 KB, patch)
2013-04-22 10:17 PDT, Mark Lam
oliver: review+
Details | Formatted Diff | Diff
benchmark result: DFG disabled using useDFGJIT()=false, watchdog disabled. (22.75 KB, text/plain)
2013-04-22 10:19 PDT, Mark Lam
no flags Details
benchmark result: run 2: DFG disabled using useDFGJIT()=false, watchdog disabled. (22.81 KB, text/plain)
2013-04-22 10:19 PDT, Mark Lam
no flags Details
benchmark result: DFG enabled, watchdog disabled. (22.62 KB, text/plain)
2013-04-22 10:20 PDT, Mark Lam
no flags Details
benchmark result: run 2: DFG enabled, watchdog disabled. (22.60 KB, text/plain)
2013-04-22 10:20 PDT, Mark Lam
no flags Details
benchmark result: DFG disabled using useDFGJIT()=false, watchdog enabled. (22.32 KB, text/plain)
2013-04-22 16:13 PDT, Mark Lam
no flags Details
benchmark result: run 2: DFG disabled using useDFGJIT()=false, watchdog enabled. (22.43 KB, text/plain)
2013-04-22 16:13 PDT, Mark Lam
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-04-22 09:36:17 PDT
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.
Comment 1 Mark Lam 2013-04-22 09:43:47 PDT
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.
Comment 2 Julien Brianceau 2013-04-22 09:50:26 PDT
(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 !
Comment 3 Mark Lam 2013-04-22 10:17:48 PDT
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.
Comment 4 Mark Lam 2013-04-22 10:19:06 PDT
Created attachment 199043 [details]
benchmark result: DFG disabled using useDFGJIT()=false, watchdog disabled.
Comment 5 Mark Lam 2013-04-22 10:19:47 PDT
Created attachment 199044 [details]
benchmark result: run 2: DFG disabled using useDFGJIT()=false, watchdog disabled.
Comment 6 WebKit Commit Bot 2013-04-22 10:20:12 PDT
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.
Comment 7 Mark Lam 2013-04-22 10:20:22 PDT
Created attachment 199045 [details]
benchmark result: DFG enabled, watchdog disabled.
Comment 8 Mark Lam 2013-04-22 10:20:46 PDT
Created attachment 199046 [details]
benchmark result: run 2: DFG enabled, watchdog disabled.
Comment 9 Mark Lam 2013-04-22 10:23:38 PDT
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”.
Comment 10 Oliver Hunt 2013-04-22 10:25:51 PDT
Comment on attachment 199042 [details]
the patch.

r=me
Comment 11 Mark Lam 2013-04-22 10:37:50 PDT
Thanks for the review.  Landed in r148893: <http://trac.webkit.org/changeset/148893>.
Comment 12 Julien Brianceau 2013-04-22 10:50:00 PDT
(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.
Comment 13 Geoffrey Garen 2013-04-22 11:57:10 PDT
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.
Comment 14 Geoffrey Garen 2013-04-22 11:57:48 PDT
Does this patch change performance when the watchdog is enabled?
Comment 15 Geoffrey Garen 2013-04-22 12:01:38 PDT
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.
Comment 16 Mark Lam 2013-04-22 16:13:08 PDT
Created attachment 199125 [details]
benchmark result: DFG disabled using useDFGJIT()=false, watchdog enabled.
Comment 17 Mark Lam 2013-04-22 16:13:54 PDT
Created attachment 199126 [details]
benchmark result: run 2: DFG disabled using useDFGJIT()=false, watchdog enabled.
Comment 18 Mark Lam 2013-04-22 16:20:40 PDT
(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.