Bug 114963

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.

(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 !

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.

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

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

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.

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

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

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 on attachment 199042 [details] the patch. r=me

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

(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 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.

Does this patch change performance when the watchdog is enabled?

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.

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

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

(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.