Bug 167737

Created attachment 300421 [details] it begins

Created attachment 300430 [details] it is written Ugh, this will not be fun to test.

Created attachment 300453 [details] more things compile

Created attachment 300458 [details] it compiles!

Created attachment 300578 [details] crashes after 1 GC

Created attachment 300581 [details] restoring the "scan my own thread" part of ConservativeRoots Well that's an annoying refactoring to have to do. I think I'm almost done though. I mostly just reverted that part of the original "GC in thread" patch.

Created attachment 300635 [details] it's so correct that it can run FIVE collections before crashing

Created attachment 300656 [details] seems to work

Created attachment 300658 [details] performs well and added tests for passing the conn It's a huge patch because it includes two version of splay.

Created attachment 300739 [details] fix more bugs

Created attachment 300769 [details] OMG it passes so many tests

Attachment 300769 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/GCConningOfficer.h:40: The parameter name "officer" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1127: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1153: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1163: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 17 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.

Perfect score!

Created attachment 300781 [details] fix build a bit

Attachment 300781 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/GCConningOfficer.h:40: The parameter name "officer" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1127: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1153: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1163: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 17 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 300782 [details] more fixes

Created attachment 300783 [details] more Fixed some inline asm

Attachment 300783 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/GCConningOfficer.h:40: The parameter name "officer" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1127: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1153: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1163: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 17 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 300786 [details] fix more build

Attachment 300786 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/GCConningOfficer.h:40: The parameter name "officer" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1127: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1153: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1163: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 17 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 301216 [details] more

Attachment 301216 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1127: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1153: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1163: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 18 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 301261 [details] fixed build again

Created attachment 301275 [details] added more tests

Attachment 301275 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1119: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1128: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1154: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1164: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 301275 [details] added more tests workers/bomb.html is crashing. I need to debug that some more.

(In reply to comment #26) > Comment on attachment 301275 [details] > added more tests > > workers/bomb.html is crashing. I need to debug that some more. It seems to become a lot more stable when I switch to the old setjmp way of getting callee saves. But that doesn't seem to be enough to make it completely stable.

Comment on attachment 301275 [details] added more tests Attachment 301275 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3070203 New failing tests: js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html workers/bomb.html

Created attachment 301282 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 301275 [details] added more tests Attachment 301275 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3070356 New failing tests: js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html workers/bomb.html

Created attachment 301285 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Created attachment 301304 [details] redid conservative scanning again It looks that we really need to setjmp to get the "register state". I'm guessing that there is something not register related that ends up in the jmp_buf. In any case, I found a way to ensure that we setjmp rarely enough that it doesn't matter that it's slow.

Attachment 301304 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1139: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1148: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1174: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1184: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 13 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #32) > Created attachment 301304 [details] > redid conservative scanning again > > It looks that we really need to setjmp to get the "register state". I'm > guessing that there is something not register related that ends up in the > jmp_buf. In any case, I found a way to ensure that we setjmp rarely enough > that it doesn't matter that it's slow. Having "fixed" this, I'm getting the same crashes, even though I'm now using setjmp. But previously, when I hacked the last patch to use setjmp, I did not get crashes. Something weird is going on.

Comment on attachment 301304 [details] redid conservative scanning again Attachment 301304 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3073743 New failing tests: workers/bomb.html

Created attachment 301308 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 301304 [details] redid conservative scanning again Attachment 301304 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3073746 New failing tests: workers/bomb.html

Created attachment 301309 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

(In reply to comment #34) > (In reply to comment #32) > > Created attachment 301304 [details] > > redid conservative scanning again > > > > It looks that we really need to setjmp to get the "register state". I'm > > guessing that there is something not register related that ends up in the > > jmp_buf. In any case, I found a way to ensure that we setjmp rarely enough > > that it doesn't matter that it's slow. > > Having "fixed" this, I'm getting the same crashes, even though I'm now using > setjmp. But previously, when I hacked the last patch to use setjmp, I did > not get crashes. > > Something weird is going on. OK, it turns out that the setjmp thing was all a red herring. This was another example of getting super unlucky: every time I turned off my custom stack scan and switched to setjmp, the bug seemed to go away. In reality, the bug has a 1/2 chance of manifesting, and so sometimes you'll have a good streak. I had good streams exactly when I used setjmp. The real bug is that GC shutdown was not working right when the collector thread thread relinquished the conn. The GC would keep running as the VM deleted things, and then the GC would crash.

Created attachment 301311 [details] more I think I fixed it.

Attachment 301311 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1193: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1219: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1229: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 301315 [details] more Fixing things.

Attachment 301315 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1193: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1219: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1229: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 301317 [details] the patch Fixed windows build issue.

Attachment 301317 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1193: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1219: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1229: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 301317 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=301317&action=review Will continue to look when I'm at a computer. > Source/JavaScriptCore/heap/Heap.cpp:1530 > + dataLog("Mutator already has the conn.\n"); Should be conditional

Comment on attachment 301317 [details] the patch Attachment 301317 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3074864 New failing tests: js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html

Created attachment 301321 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6

(In reply to comment #46) > Comment on attachment 301317 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301317&action=review > > Will continue to look when I'm at a computer. > > > Source/JavaScriptCore/heap/Heap.cpp:1530 > > + dataLog("Mutator already has the conn.\n"); > > Should be conditional Ooops! I just removed it.

Created attachment 301441 [details] the patch

Attachment 301441 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1184: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1193: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1219: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1229: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 60 files If any of these errors are false positives, please file a bug against check-webkit-style.

Whoa it passed tests. That's something.

Comment on attachment 301441 [details] the patch This seems to regress performance on low-core-count machines. I must have goofed something up.

It might be something like this: performIncrement immediately steals everything from the global mark stack, which prevents the parallel draining thread from doing its thing. I bet we need to throttle performIncrement so that it never steals the whole shared stack if there are parallel markers.

(In reply to comment #54) > It might be something like this: performIncrement immediately steals > everything from the global mark stack, which prevents the parallel draining > thread from doing its thing. > > I bet we need to throttle performIncrement so that it never steals the whole > shared stack if there are parallel markers. I found some weird stuff, but overall, it seems more iOS-specific than core-count-specific. I'm still investigating.

(In reply to comment #55) > (In reply to comment #54) > > It might be something like this: performIncrement immediately steals > > everything from the global mark stack, which prevents the parallel draining > > thread from doing its thing. > > > > I bet we need to throttle performIncrement so that it never steals the whole > > shared stack if there are parallel markers. > > I found some weird stuff, but overall, it seems more iOS-specific than > core-count-specific. I'm still investigating. Well, it now looks like I was being lied to all along. The slow-down on iOS was also a fluke. I made a bad build somehow. But in the meantime I figured out a way to tune the collector on iOS that results in a further speed-up. Prior to this patch, the old non-stochastic space-time scheduler had a bad time because of timing jitter that resulted from the collector thread and mutator thread handing off to each other. But this patch removes the hand-off: the mutator thread gets to always run. It turns out that removing the incremental mode (gcIncrementScale=0) and disabling the stochastic scheduler is a win on 2-core configurations. It's a win on iOS and on Mac. I'll keep experimenting.

Created attachment 301550 [details] more Did more iOS fixing and tuning. I still have more work to do.

Created attachment 301551 [details] patch from other computer

Created attachment 301562 [details] the patch

Attachment 301562 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1218: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1244: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1254: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 20 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 301562 [details] the patch Attachment 301562 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3118955 New failing tests: js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html

Created attachment 301571 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

(In reply to comment #61) > Comment on attachment 301562 [details] > the patch > > Attachment 301562 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/3118955 > > New failing tests: > js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html I'm investigating whether this is an actual deadlock, or if it's just that the test takes too long. Could easily be the latter. I'll shorten the test.

Created attachment 301781 [details] the patch Fixing that silly test.

Attachment 301781 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1218: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1244: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1254: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 20 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 301781 [details] the patch r=me.

Doing one more little round of testing and then I'll land.

Created attachment 301808 [details] patch for landing

Attachment 301808 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:947: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1218: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1244: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1254: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.

*** Bug 166827 has been marked as a duplicate of this bug. ***

Mac EWS sees a red tree, but I think I can land anyway because this patch has been through EWS a bunch of times already.

Landed in https://trac.webkit.org/changeset/212466

(In reply to comment #72) > Landed in https://trac.webkit.org/changeset/212466 FYI, it made 1000 JSC tests crash on AArch64, see bug168502 for details.

The LayoutTest for this change is frequently timing out on Sierra: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=js%2Fdom%2Fgc-slot-visitor-parallel-drain-pings-runloop-when-done.html

Re-opened since this is blocked by bug 168577

Created attachment 302100 [details] patch for relanding

Created attachment 302101 [details] better patch for relanding Readded changelogs, removed that silly GC runloop LayoutTest.

Attachment 302101 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:946: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1217: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1243: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1253: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.

Rolled out the Cloop build fix in https://trac.webkit.org/changeset/212624 as CLoop builders can't build WebKit with it.

Created attachment 302170 [details] more

Attachment 302170 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/RunningScope.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/CollectorPhase.h:64: The parameter name "phase" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/IncrementalSweeper.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/CommonVM.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/MachineStackMarker.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/GCFinalizationCallback.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WTF/wtf/NumberOfCores.cpp:53: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/ParallelHelperPool.h:172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:48: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:66: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:88: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/heap/RegisterState.h:115: The parameter name """" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:946: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1217: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1243: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1253: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 21 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.

Relanded in https://trac.webkit.org/changeset/212778. The problem with the previous patch was bad use of RAII scope.

The fix was re-landed without a test (js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html). Filip, is that expected?

(In reply to comment #83) > The fix was re-landed without a test > (js/dom/gc-slot-visitor-parallel-drain-pings-runloop-when-done.html). Filip, > is that expected? Yes. That test was not very valuable. And it was flaky, and needed a bunch of new infrastructure. But if the thing it was testing regressed, we would have a perf and memory regression and we would catch it anyway.