Bug 151793

For some context, see https://bugs.webkit.org/show_bug.cgi?id=151746#c15.

<rdar://problem/23737908>

First we need to unbreak the result profiling. This basically means rolling out http://trac.webkit.org/changeset/200502.

Created attachment 278283 [details] work in progress

Created attachment 278336 [details] wow that was easy Turns out that the only thing broken with ResultProfile was how ByteCodeParser handled it. It completely skipped the ResultProfile if we were not "likely to take slow case".

Created attachment 278344 [details] add more things

Created attachment 278368 [details] more... I have to get the slow paths of baseline add/sub/mul to do profiling.

Created attachment 278369 [details] omg starting to work

Created attachment 278372 [details] passing some tests

Created attachment 278411 [details] performance

Created attachment 278412 [details] works on 64-bit Now I need to port this to 32-bit. It won't be too hard.

Created attachment 278416 [details] the patch

Attachment 278416 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 3 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 278416 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=278416&action=review > Source/JavaScriptCore/bytecode/DFGExitProfile.cpp:47 > + if (false) // FIXME Ooops. I meant to add an Option for this. I just added a Options::verboseExitProfile() locally.

Created attachment 278418 [details] the patch Turns out I should make TagRegistersMode.h a private header.

Attachment 278418 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 3 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.

I counted 7 "definitely slower"s in the perf results. Are those real, or just due to noise?

Looks like some WES bot sadness too. The EFL one is definitely fixable.

(In reply to comment #17) > I counted 7 "definitely slower"s in the perf results. Are those real, or > just due to noise? I don't know. We usually ignore individual benchmark results if they contradict the aggregate scores. JSRegress's aggregate is definitely faster. The other benchmark suites' aggregate scores did not change. So, I think we don't need to pay close attention to individual benchmarks. Note that if you're using 95% confidence intervals as we are, then you should see 1/20 benchmarks report a "definitely" result even if you're comparing a build to itself. That's why it's good to err on the side of ignoring individual benchmarks unless they report a big change or the benchmark aggregate reports a change.

Looks like the test failures that the bots saw were not real. I've fixed the EFL build failure, that actually revealed a bug!

Created attachment 278431 [details] the patch Fixes a compile bug in the baseline op_sub impl

Attachment 278431 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 3 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 278435 [details] the patch Let's try this again!

Attachment 278435 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 3 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 278418 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=278418&action=review > Source/JavaScriptCore/bytecode/ValueProfile.h:255 > + void emitDetectBitsLight(CCallHelpers&, JSValueRegs, TagRegistersMode = HaveTagRegisters); > + > + void detectBitsLight(JSValue value) Why the "Light"? emitDetectBitsLight seems ambiguous and doesn't actually describe its purpose / action. How about renaming emitDetectBitsLight as emitDetectNumericness, and detectBitsLight as detectNumericness? Unfortunately, I don't have a better word for Numericness, but I think it does accurately convey the idea of what's being done by these functions. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:239 > - if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node()) > - && m_graph.hasExitSite(node->origin.semantic, BadType)) { > + if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())) { Why not do the same removal of "m_graph.hasExitSite(node->origin.semantic, BadType)" for ArithSub as well? I only mention ArithSub because you worked on the sub operator in this patch. Note: the Bit??? ops, and ArithDiv/Mod also has this potentially unneeded check. > Source/JavaScriptCore/jit/AssemblyHelpers.h:781 > + Jump branchIfNotDoubleKnownNotInt32(JSValueRegs regs, TagRegistersMode mode = HaveTagRegisters) It looks like what this code is branching if the regs contains a non-numeric JSValue. Why not name this emitter method branchIfNotNumber instead? I see that there is already a branchIfNotNumber() and its 64-bit implementation is identical to this one. Ah, based on the 32-bit impl, I see what you mean by "IfNotDouble" and "KnownNotInt32". If this is emitter is not crucial to perf, I suggest just removing this and just using branchIfNotNumber instead. > Source/JavaScriptCore/jit/JITArithmetic.cpp:991 > + ResultProfile* resultProfile = nullptr; > + if (shouldEmitProfiling()) > + resultProfile = m_codeBlock->ensureResultProfile(m_bytecodeOffset); You forgot to plug the resultProfile into the JITSubGenerator below. You're currently not detecting double results on the fast path.

(In reply to comment #25) > Comment on attachment 278418 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278418&action=review > > > Source/JavaScriptCore/bytecode/ValueProfile.h:255 > > + void emitDetectBitsLight(CCallHelpers&, JSValueRegs, TagRegistersMode = HaveTagRegisters); > > + > > + void detectBitsLight(JSValue value) > > Why the "Light"? emitDetectBitsLight seems ambiguous and doesn't actually > describe its purpose / action. How about renaming emitDetectBitsLight as > emitDetectNumericness, and detectBitsLight as detectNumericness? > Unfortunately, I don't have a better word for Numericness, but I think it > does accurately convey the idea of what's being done by these functions. I like emitDetectNumericness and detectNumericness. I will use that. > > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:239 > > - if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node()) > > - && m_graph.hasExitSite(node->origin.semantic, BadType)) { > > + if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())) { > > Why not do the same removal of "m_graph.hasExitSite(node->origin.semantic, > BadType)" for ArithSub as well? I only mention ArithSub because you worked > on the sub operator in this patch. Note: the Bit??? ops, and ArithDiv/Mod > also has this potentially unneeded check. Oh wow! I will fix this for ArithSub. This patch doesn't do anything for div/mod, so I will leave those unchanged. I think that we should leave div/mod for after this lands and stabilises. > > > Source/JavaScriptCore/jit/AssemblyHelpers.h:781 > > + Jump branchIfNotDoubleKnownNotInt32(JSValueRegs regs, TagRegistersMode mode = HaveTagRegisters) > > It looks like what this code is branching if the regs contains a non-numeric > JSValue. Why not name this emitter method branchIfNotNumber instead? I see > that there is already a branchIfNotNumber() and its 64-bit implementation is > identical to this one. Ah, based on the 32-bit impl, I see what you mean by > "IfNotDouble" and "KnownNotInt32". > > If this is emitter is not crucial to perf, I suggest just removing this and > just using branchIfNotNumber instead. It saves a register, which is important in a bunch of places. > > > Source/JavaScriptCore/jit/JITArithmetic.cpp:991 > > + ResultProfile* resultProfile = nullptr; > > + if (shouldEmitProfiling()) > > + resultProfile = m_codeBlock->ensureResultProfile(m_bytecodeOffset); > > You forgot to plug the resultProfile into the JITSubGenerator below. You're > currently not detecting double results on the fast path. Fixed!

Comment on attachment 278435 [details] the patch r=me with fixes, and if perf does not regress after the ArithSub OSR exit check is removed. You mentioned leaving ArithMod/Div alone. I presume you'll leave the Bit ops alone too since you didn't touch those in this patch?

(In reply to comment #26) > (In reply to comment #25) > > Comment on attachment 278418 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=278418&action=review > > > > > Source/JavaScriptCore/bytecode/ValueProfile.h:255 > > > + void emitDetectBitsLight(CCallHelpers&, JSValueRegs, TagRegistersMode = HaveTagRegisters); > > > + > > > + void detectBitsLight(JSValue value) > > > > Why the "Light"? emitDetectBitsLight seems ambiguous and doesn't actually > > describe its purpose / action. How about renaming emitDetectBitsLight as > > emitDetectNumericness, and detectBitsLight as detectNumericness? > > Unfortunately, I don't have a better word for Numericness, but I think it > > does accurately convey the idea of what's being done by these functions. > > I like emitDetectNumericness and detectNumericness. I will use that. Fixed. > > > > > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:239 > > > - if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node()) > > > - && m_graph.hasExitSite(node->origin.semantic, BadType)) { > > > + if (Node::shouldSpeculateUntypedForArithmetic(leftChild.node(), rightChild.node())) { > > > > Why not do the same removal of "m_graph.hasExitSite(node->origin.semantic, > > BadType)" for ArithSub as well? I only mention ArithSub because you worked > > on the sub operator in this patch. Note: the Bit??? ops, and ArithDiv/Mod > > also has this potentially unneeded check. > > Oh wow! I will fix this for ArithSub. > > This patch doesn't do anything for div/mod, so I will leave those unchanged. > I think that we should leave div/mod for after this lands and stabilises. Fixed. > > > > > > Source/JavaScriptCore/jit/AssemblyHelpers.h:781 > > > + Jump branchIfNotDoubleKnownNotInt32(JSValueRegs regs, TagRegistersMode mode = HaveTagRegisters) > > > > It looks like what this code is branching if the regs contains a non-numeric > > JSValue. Why not name this emitter method branchIfNotNumber instead? I see > > that there is already a branchIfNotNumber() and its 64-bit implementation is > > identical to this one. Ah, based on the 32-bit impl, I see what you mean by > > "IfNotDouble" and "KnownNotInt32". > > > > If this is emitter is not crucial to perf, I suggest just removing this and > > just using branchIfNotNumber instead. > > It saves a register, which is important in a bunch of places. > > > > > > Source/JavaScriptCore/jit/JITArithmetic.cpp:991 > > > + ResultProfile* resultProfile = nullptr; > > > + if (shouldEmitProfiling()) > > > + resultProfile = m_codeBlock->ensureResultProfile(m_bytecodeOffset); > > > > You forgot to plug the resultProfile into the JITSubGenerator below. You're > > currently not detecting double results on the fast path. > > Fixed!

Created attachment 278457 [details] patch for landing I still need to test performance. Putting up for EWS.

Attachment 278457 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:131: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 3 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 278470 [details] performance

Landed in http://trac.webkit.org/changeset/200606

This broke the CLoop build: https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100

(In reply to comment #33) > This broke the CLoop build: > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100 Looking!

(In reply to comment #34) > (In reply to comment #33) > > This broke the CLoop build: > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100 > > Looking! I have a fix, will land shortly.

(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > This broke the CLoop build: > > > https://build.webkit.org/builders/ > > > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29?numbuilds=100 > > > > Looking! > > I have a fix, will land shortly. Fixed in http://trac.webkit.org/changeset/200613.

Looks like a 1.7% on Dromaeo DOM Core benchmark on MacBookAir.

(In reply to comment #37) > Looks like a 1.7% on Dromaeo DOM Core benchmark on MacBookAir. I forgot the *key* word: 1.7% *PROGRESSION*!