Bug 152133

Summary: Math.random should have an intrinsic thunk and it should be later handled as a DFG Node
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, rniwa, saam, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Patch ggaren: review+

Description Yusuke Suzuki 2015-12-10 09:05:55 PST
...
Comment 1 Yusuke Suzuki 2015-12-12 07:36:02 PST
Created attachment 267232 [details]
Patch

64bit
Comment 2 Yusuke Suzuki 2015-12-12 10:16:59 PST
Created attachment 267233 [details]
Patch
Comment 3 WebKit Commit Bot 2015-12-12 10:18:26 PST
Attachment 267233 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:481:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:484:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:487:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:490:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:504:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:507:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:510:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:513:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Yusuke Suzuki 2015-12-12 10:24:42 PST
Created attachment 267234 [details]
Patch
Comment 5 Yusuke Suzuki 2015-12-12 10:26:53 PST
And performance.

math-random                                    37.4632+-1.9935     ^     13.3766+-1.2503        ^ definitely 2.8006x faster
ftl-library-inlining                          159.3640+-10.8216    ^     15.8363+-0.9978        ^ definitely 10.0632x faster

Second case is unintentional. In the second case, we use Math.random as a representative of library function.
But now, it's inlined.
Comment 6 WebKit Commit Bot 2015-12-12 10:27:29 PST
Attachment 267234 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:481:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:484:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:487:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:490:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:504:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:507:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:510:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:513:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2015-12-12 10:58:36 PST
Comment on attachment 267234 [details]
Patch

Oops. Still crashing.
Comment 8 Build Bot 2015-12-12 11:29:40 PST
Comment on attachment 267234 [details]
Patch

Attachment 267234 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/550644

New failing tests:
js/regress/math-random.html
js/sort-randomly.html
js/slow-stress/nested-function-parsing-random.html
Comment 9 Build Bot 2015-12-12 11:29:44 PST
Created attachment 267235 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2015-12-12 11:32:44 PST
Comment on attachment 267234 [details]
Patch

Attachment 267234 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/550642

New failing tests:
js/regress/math-random.html
js/slow-stress/nested-function-parsing-random.html
Comment 11 Build Bot 2015-12-12 11:32:48 PST
Created attachment 267236 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 12 Build Bot 2015-12-12 11:33:39 PST
Comment on attachment 267234 [details]
Patch

Attachment 267234 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/550639

New failing tests:
js/regress/math-random.html
js/sort-randomly.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-L_S.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-B2.html
js/slow-stress/nested-function-parsing-random.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
Comment 13 Build Bot 2015-12-12 11:33:42 PST
Created attachment 267237 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 14 Yusuke Suzuki 2015-12-12 12:05:06 PST
Clang generated code.

 50     double get()                                                                
 51     {                                                                           
 52         uint64_t value = advance() & ((1ULL << 53) - 1);                        
 53         return value / static_cast<double>(1ULL << 53);                         
 54     } 

to

.LBB0_3:
	movq	_ZZ5hellovE12staticRandom+8(%rip), %rax
	movq	_ZZ5hellovE12staticRandom+16(%rip), %rcx
	movq	%rcx, _ZZ5hellovE12staticRandom+8(%rip)
	movq	%rax, %rdx
	shlq	$23, %rdx
	xorq	%rax, %rdx
	movq	%rcx, %rax
	shrq	$26, %rax
	xorq	%rcx, %rax
	xorq	%rdx, %rax
	shrq	$17, %rdx
	xorq	%rax, %rdx
	movq	%rdx, _ZZ5hellovE12staticRandom+16(%rip)
	addq	%rcx, %rdx
	movabsq	$9007199254740991, %rax # imm = 0x1FFFFFFFFFFFFF
	andq	%rdx, %rax
	cvtsi2sdq	%rax, %xmm0
	mulsd	.LCPI0_1(%rip), %xmm0
Comment 15 Yusuke Suzuki 2015-12-12 12:07:28 PST
Created attachment 267239 [details]
Patch
Comment 16 WebKit Commit Bot 2015-12-12 12:08:25 PST
Attachment 267239 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:480:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:483:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:486:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:489:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:503:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:506:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:509:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:512:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2015-12-12 13:12:50 PST
Comment on attachment 267239 [details]
Patch

Attachment 267239 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/550935

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-L_S.html
Comment 18 Build Bot 2015-12-12 13:12:55 PST
Created attachment 267240 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 19 Yusuke Suzuki 2015-12-12 13:37:41 PST
Created attachment 267241 [details]
Patch
Comment 20 WebKit Commit Bot 2015-12-12 13:40:31 PST
Attachment 267241 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:480:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:483:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:486:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:489:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:503:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:506:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:509:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:512:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Geoffrey Garen 2015-12-12 14:11:55 PST
Comment on attachment 267241 [details]
Patch

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

Patch looks good to me. I think Phil needs to review the change to DFGClobberize.h.

Please run the convenient subset of distribution tests cited @ http://jandemooij.nl/blog/2015/11/30/testing-math-random-crushing-the-browser/, http://jandemooij.nl/blog/2015/11/27/math-random-and-32-bit-precision/.

Oliver mentioned an interesting related idea: We could use the Intel random instruction for the inline assembly path. It's OK if the algorithm for randomness changes based engine -- it need not be deterministic.

> Source/JavaScriptCore/dfg/DFGClobberize.h:185
> +    case ArithRandom:
> +        // Even World is clobbered, ArithRandom works. So we don't note as `read(MiscFields)`.
> +        // But we note it as `write(SideState)` and it produces an impure value.
> +        // It prevent hoisting ArithRandom. For example,
> +        //
> +        //     var ok = false;
> +        //     for (var i = 0; i < 10000; ++i) {
> +        //         if (Math.random() < 0.5)
> +        //             ok = true;
> +        //     }
> +        //
> +        // In the above case, Hoisting `Math.random()` is not acceptable. But,
> +        //
> +        //     for (var i = 0; i < 10000; ++i) Math.random();
> +        //
> +        // In the above case, eliminating `Math.random()` is OK since RNG internal state is not observable to users.
> +        // So ArithRandom node is not `MustGenerate`.
> +        write(SideState);
> +        return;

I'd like Phil to review this part.

> Source/WTF/wtf/WeakRandom.h:64
> +        uint64_t value = advance() & ((1ULL << 53) - 1);
> +        return value * (1.0 / (1ULL << 53));

I think this deserves a comment that there are only 53 bits of precision in a double.

You should also add a comment to the class that says that it needs to stay in sync with the JIT implementation.
Comment 22 Filip Pizlo 2015-12-12 18:17:29 PST
Comment on attachment 267241 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGClobberize.h:185
>> +        return;
> 
> I'd like Phil to review this part.

The canonical way to do this would be to introduce a MathDotRandomState abstract heap, and read+write it.  There are two reasons to do it this way.  The first is simple: we do it that way because it happens to be the truth: there exists some state in memory that Math.random will both read and write.  There's an old Mark Twain quote that I like to apply to the analyses in the compiler: "If you tell the truth, you don't have to remember anything."  In other words, if you write the analysis to tell the truth about what is going on, then you don't have to think about how the rest of the compiler will use your analysis.

What you have done is analyzed what the compiler will do with your call to write(SideState) and concluded that this is enough.  There are some things you overlooked:

- We don't want the behavior of the random number generator to change when the code is optimized.  The number of calls to Math.random() is observable, since enough such calls could cause the PRNG to repeat itself.  So, dead-code-elimination of ArithRandom is unsound.  Please make ArithRandom be MustGenerate.
- There is no connection between producing a pure value and writing SideState.  A node produces a pure value only if you do def(PureValue(...)).
- SideState is OSR state.  The compiler is free to assume that if it has already separately analyzed OSR effects, then it can ignore all writes to SideState.  One example of this is clobbersExitState(), which tries to determine if OSR would be observable if it executed after the node.  It assumes that writes to SideState don't affect OSR observability, because that analysis is used by clients that separately track OSR state.  So, currently, your formulation will cause the compiler to assume that it's OK to exit right after an ArithRandom, which will cause the Math.random() call to be executed a second time after OSR.
- I don't know what you mean by "Even [if] World is clobbered, ArithRandom works".  Even if you read(World)/write(World), ArithRandom will still work just fine.  We don't care about whether someone else clobbered World, since we aren't doing load elimination or CSE on Math.random() calls.  So, you could read(World) and it would not affect any optimizations.  But, as I suggest above, the simple thing to do is to just tell the truth: Math.random() indeed has some state that it both reads and writes, so abstract that state as an AbstractHeap and then read+write it.

There may be other things that you overlooked.  Because your justification relies on an analysis of users of clobberize(), we would actually need to exhaustively audit the whole compiler to confirm if what you're doing is right.  It's not worth it.  That's why for new nodes, we write the clobberize() rule by strictly modeling exactly what the Node actually does.  If it reads things, then you need a read() call and if it writes things then you need a write() call.  In this case, doing so will not inhibit any optimizations, so being clever isn't worth it.

In future, it's best to obey the rules of clobberize() without considering how it's called.  If your Node can write to memory for any reason whatsoever, then you need a write() to some abstract heap and that abstract heap should not be SideState.  If your Node can read memory and that memory is not immutable, then you need a read() to the appropriate abstract heap.  It's important to do this because the DFG compiler is very large and has many optimizations that use clobberize().  It would be impractical to always consider all of the users of clobberize() when writing clobberize() rules, so it's much better to just obey the established contract.
Comment 23 Yusuke Suzuki 2015-12-13 04:27:49 PST
Comment on attachment 267241 [details]
Patch

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

Thanks for your insightful reviews!
I'll upload the revised patch once I changed it and ensured that our new implementation can pass the tests in http://jandemooij.nl/blog/2015/11/30/testing-math-random-crushing-the-browser/ http://jandemooij.nl/blog/2015/11/27/math-random-and-32-bit-precision/

>>> Source/JavaScriptCore/dfg/DFGClobberize.h:185
>>> +        return;
>> 
>> I'd like Phil to review this part.
> 
> The canonical way to do this would be to introduce a MathDotRandomState abstract heap, and read+write it.  There are two reasons to do it this way.  The first is simple: we do it that way because it happens to be the truth: there exists some state in memory that Math.random will both read and write.  There's an old Mark Twain quote that I like to apply to the analyses in the compiler: "If you tell the truth, you don't have to remember anything."  In other words, if you write the analysis to tell the truth about what is going on, then you don't have to think about how the rest of the compiler will use your analysis.
> 
> What you have done is analyzed what the compiler will do with your call to write(SideState) and concluded that this is enough.  There are some things you overlooked:
> 
> - We don't want the behavior of the random number generator to change when the code is optimized.  The number of calls to Math.random() is observable, since enough such calls could cause the PRNG to repeat itself.  So, dead-code-elimination of ArithRandom is unsound.  Please make ArithRandom be MustGenerate.
> - There is no connection between producing a pure value and writing SideState.  A node produces a pure value only if you do def(PureValue(...)).
> - SideState is OSR state.  The compiler is free to assume that if it has already separately analyzed OSR effects, then it can ignore all writes to SideState.  One example of this is clobbersExitState(), which tries to determine if OSR would be observable if it executed after the node.  It assumes that writes to SideState don't affect OSR observability, because that analysis is used by clients that separately track OSR state.  So, currently, your formulation will cause the compiler to assume that it's OK to exit right after an ArithRandom, which will cause the Math.random() call to be executed a second time after OSR.
> - I don't know what you mean by "Even [if] World is clobbered, ArithRandom works".  Even if you read(World)/write(World), ArithRandom will still work just fine.  We don't care about whether someone else clobbered World, since we aren't doing load elimination or CSE on Math.random() calls.  So, you could read(World) and it would not affect any optimizations.  But, as I suggest above, the simple thing to do is to just tell the truth: Math.random() indeed has some state that it both reads and writes, so abstract that state as an AbstractHeap and then read+write it.
> 
> There may be other things that you overlooked.  Because your justification relies on an analysis of users of clobberize(), we would actually need to exhaustively audit the whole compiler to confirm if what you're doing is right.  It's not worth it.  That's why for new nodes, we write the clobberize() rule by strictly modeling exactly what the Node actually does.  If it reads things, then you need a read() call and if it writes things then you need a write() call.  In this case, doing so will not inhibit any optimizations, so being clever isn't worth it.
> 
> In future, it's best to obey the rules of clobberize() without considering how it's called.  If your Node can write to memory for any reason whatsoever, then you need a write() to some abstract heap and that abstract heap should not be SideState.  If your Node can read memory and that memory is not immutable, then you need a read() to the appropriate abstract heap.  It's important to do this because the DFG compiler is very large and has many optimizations that use clobberize().  It would be impractical to always consider all of the users of clobberize() when writing clobberize() rules, so it's much better to just obey the established contract.

Thank you for your clarification! It's very very insightful :D

I completely agree to your opinion about compiler's analysis - "If you tell the truth, you don't have to remember anything." In other words, if you write the analysis to tell the truth about what is going on, then you don't have to think about how the rest of the compiler will use your analysis.
It makes each analysis solid and easy to compose a compiler of them.

And after reading your review, I changed my mind; Math.random function call is observable to users.
In the previous patch, I created the patch based on the thought that PRNG's internal states should not be observable to users because PRNG should have period long enough to avoid user's observability.
But even if the period of the PRNG is long enough, it has the state and Math.random call changes it. So in the upcoming patch, I change this. Math.random is now observable behavior. And clobberize phase should report the side effects of ArithRandom precisely.

And thank you for your clarification about SideState. I didn't know SideState means OSRState. I read DFGAbstractHeap.h / DFGClobbersExitState.cpp to ensure that SideState side effect is not considered when executing OSR Exit. This was OK for ArithRandom in the first patch.



I'll change the patch to use MathDotRandomState abstract heap. Change ArithRandom's attribute to MustGenerate.
And in the revised patch, ArithRandom in clobberize reports its read/write precisely.
And now, I just use the current logic to generate random numbers instead of using x86 RNG instruction (RDRAND). Because by using RDRAND we indeterministicly change our RNG implementation, it causes the similar problem to the number of Math.random calls. But it's worth trying to always use RDRAND if it's supported. But in this case, we cannot provide PRNG seed to x86 PRNG and PRNG state is shared.

The following is the note about the first patch I submitted.
In the first patch, I attempted to solve the problem with the following considerations.

1. PRNG state (almost) should not be observable to users because PRNG should have enough long period (In this case, Xorshift+128's period is 2**128 − 1). So how many times calling it is not observable. Based on this assumption, I (1) dropped `MustGenerate` to allow DFG to elliminate it, and (2) note it as `write(SideState)` since it was OK to ArithRandom to be re-executed after OSRExit. Seeing "DFGClobbersExitState.cpp", SideState and its subtype were the only write kind that is allowed to be re-executed.
2. "There is no connection between producing a pure value and writing SideState." Oops! Sorry, it's confusing statement. "it produces an impure value" part is just the explanation why I did not write `def(PureValue(node))` here.
3. `read(World)` is dropped to represent that `ArithRandom` does not rely on the World's status. `ArithRandom` can be moved over `write(World)` node.

>> Source/WTF/wtf/WeakRandom.h:64
>> +        return value * (1.0 / (1ULL << 53));
> 
> I think this deserves a comment that there are only 53 bits of precision in a double.
> 
> You should also add a comment to the class that says that it needs to stay in sync with the JIT implementation.

Thanks! I've added it.
Comment 24 Yusuke Suzuki 2015-12-13 06:22:49 PST
Created attachment 267263 [details]
Patch
Comment 25 Yusuke Suzuki 2015-12-13 09:33:08 PST
Comment on attachment 267263 [details]
Patch

OK, the patch is ready.
I've checked this on http://jandemooij.nl/test/random-precision.htm.
But I cannot run Crush tests (on GTK+ port MiniBrowser even without this patch), this should be later investigated.
Comment 26 WebKit Commit Bot 2015-12-13 09:33:13 PST
Attachment 267263 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:480:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:483:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:486:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:489:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:503:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:506:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:509:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:512:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Geoffrey Garen 2015-12-14 12:49:49 PST
> There's an old Mark Twain quote that I like to apply to the analyses in the
> compiler: "If you tell the truth, you don't have to remember anything."

LOL, I've spent my whole life thinking that my grandfather made up this saying! I guess Mark Twain is kind of like America's grandfather.
Comment 28 Geoffrey Garen 2015-12-14 12:51:11 PST
Comment on attachment 267263 [details]
Patch

r=me

Did the speedup magnitude change, now that the node is MustGenerate?
Comment 29 Yusuke Suzuki 2015-12-14 18:49:53 PST
(In reply to comment #28)
> Comment on attachment 267263 [details]
> Patch
> 
> r=me
> 
> Did the speedup magnitude change, now that the node is MustGenerate?

I've just measured :D

math-random                                       41.3322+-4.8054     ^     15.1425+-0.5141        ^ definitely 2.7295x faster
  ftl-library-inlining                             177.4745+-19.1112    ^     31.6082+-9.1560        ^ definitely 5.6148x faster

math-random result is not changed because we carefully crafted math-random test to avoid eliminating ArithRandom.
ftl-library-inlining optimization becomes small. In the previous case, since the ArithRandom of ftl-library-inlining is not used, ArithRandom in this test is completely removed. It is expected :)
Comment 30 Yusuke Suzuki 2015-12-14 19:52:12 PST
Committed r194087: <http://trac.webkit.org/changeset/194087>
Comment 31 Yusuke Suzuki 2015-12-14 20:09:17 PST
WinCairo bot fails. But it seems strange.

FAILED: C:\PROGRA~2\MICROS~3.0\VC\bin\amd64\cl.exe   /nologo /TP /DWIN32 /D_WINDOWS /W4 /GR- /EHs- /EHc-  /MT /O2 /Ob2 /D NDEBUG -IDerivedSources\ForwardingHeaders -IDerivedSources -I..\..\WebKitLibraries\win\include -I. -I..\..\Source\JavaScriptCore -I..\..\Source\JavaScriptCore\.. -I..\..\Source\JavaScriptCore\API -I..\..\Source\JavaScriptCore\ForwardingHeaders -I..\..\Source\JavaScriptCore\assembler -I..\..\Source\JavaScriptCore\b3 -I..\..\Source\JavaScriptCore\b3\air -I..\..\Source\JavaScriptCore\bindings -I..\..\Source\JavaScriptCore\builtins -I..\..\Source\JavaScriptCore\bytecode -I..\..\Source\JavaScriptCore\bytecompiler -I..\..\Source\JavaScriptCore\dfg -I..\..\Source\JavaScriptCore\disassembler -I..\..\Source\JavaScriptCore\ftl -I..\..\Source\JavaScriptCore\heap -I..\..\Source\JavaScriptCore\debugger -I..\..\Source\JavaScriptCore\inspector -I..\..\Source\JavaScriptCore\inspector\agents -I..\..\Source\JavaScriptCore\inspector\augmentable -I..\..\Source\JavaScriptCore\inspector\remote -I..\..\Source\JavaScriptCore\interpreter -I..\..\Source\JavaScriptCore\jit -I..\..\Source\JavaScriptCore\llint -I..\..\Source\JavaScriptCore\llvm -I..\..\Source\JavaScriptCore\parser -I..\..\Source\JavaScriptCore\profiler -I..\..\Source\JavaScriptCore\replay -I..\..\Source\JavaScriptCore\runtime -I..\..\Source\JavaScriptCore\tools -I..\..\Source\JavaScriptCore\wasm -I..\..\Source\JavaScriptCore\yarr -IDerivedSources\JavaScriptCore -IDerivedSources\JavaScriptCore\inspector -I..\include\private    /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4702 /wd4706 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4702 /wd4706 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /Gy- /openmp- /GF- /Oy- /showIncludes -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DNOMINMAX -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WINDOWS /FoSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\jit\AssemblyHelpers.cpp.obj /FdSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\ /FS -c ..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(477): error C2039: 'lowOffset': is not a member of 'WTF::WeakRandom'
C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/WeakRandom.h(40): note: see declaration of 'WTF::WeakRandom'
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(477): error C3861: 'lowOffset': identifier not found
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(478): error C2039: 'highOffset': is not a member of 'WTF::WeakRandom'
C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/WeakRandom.h(40): note: see declaration of 'WTF::WeakRandom'
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(478): error C3861: 'highOffset': identifier not found
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(504): error C2039: 'highOffset': is not a member of 'WTF::WeakRandom'
C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/WeakRandom.h(40): note: see declaration of 'WTF::WeakRandom'
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(504): error C3861: 'highOffset': identifier not found
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(507): error C2039: 'highOffset': is not a member of 'WTF::WeakRandom'
C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/WeakRandom.h(40): note: see declaration of 'WTF::WeakRandom'
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(507): error C3861: 'highOffset': identifier not found
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(510): error C2039: 'lowOffset': is not a member of 'WTF::WeakRandom'
C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/WeakRandom.h(40): note: see declaration of 'WTF::WeakRandom'
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(510): error C3861: 'lowOffset': identifier not found
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(513): error C2039: 'lowOffset': is not a member of 'WTF::WeakRandom'
C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/WeakRandom.h(40): note: see declaration of 'WTF::WeakRandom'
..\..\Source\JavaScriptCore\jit\AssemblyHelpers.cpp(513): error C3861: 'lowOffset': identifier not found

But WTF::WeakRandom::{highOffset,lowOffset} is correctly added...
Comment 32 Yusuke Suzuki 2015-12-14 20:15:42 PST
It seems that forwarding headers are not cleaned up.
I've requested force-clean-build.