WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
152133
Math.random should have an intrinsic thunk and it should be later handled as a DFG Node
https://bugs.webkit.org/show_bug.cgi?id=152133
Summary
Math.random should have an intrinsic thunk and it should be later handled as ...
Yusuke Suzuki
Reported
2015-12-10 09:05:55 PST
...
Attachments
Patch
(26.64 KB, patch)
2015-12-12 07:36 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(33.97 KB, patch)
2015-12-12 10:16 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(33.97 KB, patch)
2015-12-12 10:24 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-yosemite
(976.13 KB, application/zip)
2015-12-12 11:29 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2
(1.28 MB, application/zip)
2015-12-12 11:32 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews117 for mac-yosemite
(1.21 MB, application/zip)
2015-12-12 11:33 PST
,
Build Bot
no flags
Details
Patch
(33.77 KB, patch)
2015-12-12 12:07 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews112 for mac-yosemite
(1009.59 KB, application/zip)
2015-12-12 13:12 PST
,
Build Bot
no flags
Details
Patch
(33.71 KB, patch)
2015-12-12 13:37 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(34.08 KB, patch)
2015-12-13 06:22 PST
,
Yusuke Suzuki
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2015-12-12 07:36:02 PST
Created
attachment 267232
[details]
Patch 64bit
Yusuke Suzuki
Comment 2
2015-12-12 10:16:59 PST
Created
attachment 267233
[details]
Patch
WebKit Commit Bot
Comment 3
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.
Yusuke Suzuki
Comment 4
2015-12-12 10:24:42 PST
Created
attachment 267234
[details]
Patch
Yusuke Suzuki
Comment 5
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.
WebKit Commit Bot
Comment 6
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.
Yusuke Suzuki
Comment 7
2015-12-12 10:58:36 PST
Comment on
attachment 267234
[details]
Patch Oops. Still crashing.
Build Bot
Comment 8
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
Build Bot
Comment 9
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
Build Bot
Comment 10
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
Build Bot
Comment 11
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
Build Bot
Comment 12
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
Build Bot
Comment 13
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
Yusuke Suzuki
Comment 14
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
Yusuke Suzuki
Comment 15
2015-12-12 12:07:28 PST
Created
attachment 267239
[details]
Patch
WebKit Commit Bot
Comment 16
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.
Build Bot
Comment 17
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
Build Bot
Comment 18
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
Yusuke Suzuki
Comment 19
2015-12-12 13:37:41 PST
Created
attachment 267241
[details]
Patch
WebKit Commit Bot
Comment 20
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.
Geoffrey Garen
Comment 21
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.
Filip Pizlo
Comment 22
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.
Yusuke Suzuki
Comment 23
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.
Yusuke Suzuki
Comment 24
2015-12-13 06:22:49 PST
Created
attachment 267263
[details]
Patch
Yusuke Suzuki
Comment 25
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.
WebKit Commit Bot
Comment 26
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.
Geoffrey Garen
Comment 27
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.
Geoffrey Garen
Comment 28
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?
Yusuke Suzuki
Comment 29
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 :)
Yusuke Suzuki
Comment 30
2015-12-14 19:52:12 PST
Committed
r194087
: <
http://trac.webkit.org/changeset/194087
>
Yusuke Suzuki
Comment 31
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...
Yusuke Suzuki
Comment 32
2015-12-14 20:15:42 PST
It seems that forwarding headers are not cleaned up. I've requested force-clean-build.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug