Bug 163562

Summary: The GC should be in a thread
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, ggaren, jsbell, keith_miller, mark.lam, msaboff, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=164588
Bug Depends on: 163576, 163738, 163802, 163947    
Bug Blocks: 149432    
Attachments:
Description Flags
it's not much but it's a start
none
maybe this could work
none
more
none
it compiles!
none
getting further
none
for some reason it actually runs splay
none
shutting down a VM doesn't crash anymore
none
it works better than I'd expect
none
and now, with a changelog!
none
it might work
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
another attempt
none
fix another crash
none
fix another crash
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Archive of layout-test-results from ews102 for mac-yosemite
none
it's getting more interesting
none
fixing more bugs
none
more fixes
none
fixing more things
buildbot: commit-queue-
Archive of layout-test-results from ews112 for mac-yosemite
none
even more fixes
buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-yosemite
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
the most correct patch yet!
none
fixed another deadlock
buildbot: commit-queue-
Archive of layout-test-results from ews116 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews103 for mac-yosemite
none
fix more things
buildbot: commit-queue-
Archive of layout-test-results from ews116 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews102 for mac-yosemite
none
starting to make the runloop hacks work
none
HeapTimer hack compiles
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
with ThreadSpecific fixes
none
rebased patch
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews115 for mac-yosemite
none
try to fix ios build
buildbot: commit-queue-
Archive of layout-test-results from ews113 for mac-yosemite
none
rebased
buildbot: commit-queue-
Archive of layout-test-results from ews112 for mac-yosemite
none
Archive of layout-test-results from ews101 for mac-yosemite
none
the patch
none
the patch
none
the patch
ggaren: review+
patch for landing none

Description Filip Pizlo 2016-10-17 13:31:00 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-10-19 13:41:50 PDT
I got the annoying part out of the way first: I wanted to make sure that when I added a thread, that thread would not add to our thread footprint. So, I added an AutomaticThread abstraction that makes it easy to shut down a thread automatically if it's inactive. I validated the abstraction by switching all of JSC's threads over to it.

Now I can add the new GC thread without feeling guilty about it.
Comment 2 Filip Pizlo 2016-10-22 14:12:30 PDT
Created attachment 292508 [details]
it's not much but it's a start

I'm starting to figure out the basics of how to schedule the GC thread.
Comment 3 Filip Pizlo 2016-10-22 16:18:22 PDT
Created attachment 292516 [details]
maybe this could work
Comment 4 Filip Pizlo 2016-10-22 21:52:43 PDT
Created attachment 292534 [details]
more
Comment 5 Filip Pizlo 2016-10-22 22:11:54 PDT
Created attachment 292537 [details]
it compiles!
Comment 6 Filip Pizlo 2016-10-22 23:09:30 PDT
Created attachment 292540 [details]
getting further

It still can't GC without crashing.
Comment 7 Filip Pizlo 2016-10-23 10:22:09 PDT
Created attachment 292549 [details]
for some reason it actually runs splay

Holy cow this is working.
Comment 8 Filip Pizlo 2016-10-23 10:22:54 PDT
(In reply to comment #7)
> Created attachment 292549 [details]
> for some reason it actually runs splay
> 
> Holy cow this is working.

Except that it sometimes starts using more memory for seemingly no reason.  There are definitely still problems.
Comment 9 Filip Pizlo 2016-10-23 11:12:57 PDT
Created attachment 292550 [details]
shutting down a VM doesn't crash anymore
Comment 10 Filip Pizlo 2016-10-23 11:13:25 PDT
Comment on attachment 292550 [details]
shutting down a VM doesn't crash anymore

I think I also fixed the memory use problems.
Comment 11 WebKit Commit Bot 2016-10-23 13:07:28 PDT
Attachment 292550 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1005:  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:1108:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Filip Pizlo 2016-10-23 13:23:42 PDT
Created attachment 292552 [details]
it works better than I'd expect

It basically passes most tests.
Comment 13 Filip Pizlo 2016-10-23 13:26:06 PDT
Created attachment 292553 [details]
and now, with a changelog!
Comment 14 Filip Pizlo 2016-10-23 16:02:36 PDT
Created attachment 292561 [details]
it might work

Fixing more race conditions as I find them.
Comment 15 WebKit Commit Bot 2016-10-23 16:04:09 PDT
Attachment 292561 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1005:  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:1108:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Build Bot 2016-10-23 17:07:17 PDT
Comment on attachment 292561 [details]
it might work

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

Number of test failures exceeded the failure limit.
Comment 17 Build Bot 2016-10-23 17:07:22 PDT
Created attachment 292565 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 18 Build Bot 2016-10-23 17:19:10 PDT
Comment on attachment 292561 [details]
it might work

Attachment 292561 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2353441

Number of test failures exceeded the failure limit.
Comment 19 Build Bot 2016-10-23 17:19:14 PDT
Created attachment 292566 [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
Comment 20 Filip Pizlo 2016-10-23 18:19:15 PDT
I think all of those crashes are just because WorkerScriptController needs to grab the JSLock before enabling the watchdog.
Comment 21 Filip Pizlo 2016-10-23 18:58:41 PDT
Created attachment 292571 [details]
another attempt
Comment 22 Filip Pizlo 2016-10-23 19:45:39 PDT
Created attachment 292572 [details]
fix another crash

Added another JSLockHolder.
Comment 23 WebKit Commit Bot 2016-10-23 19:47:57 PDT
Attachment 292572 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1005:  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:1108:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Filip Pizlo 2016-10-23 19:59:03 PDT
Created attachment 292573 [details]
fix another crash
Comment 25 WebKit Commit Bot 2016-10-23 20:01:03 PDT
Attachment 292573 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1005:  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:1108:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Build Bot 2016-10-23 21:26:01 PDT
Comment on attachment 292573 [details]
fix another crash

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

New failing tests:
http/tests/workers/text-encoding.html
workers/bomb.html
storage/indexeddb/database-wrapper-private.html
inspector/debugger/pause-reason.html
storage/indexeddb/database-wrapper.html
inspector/debugger/removeBreakpoint.html
inspector/debugger/breakpoint-eval-with-exception.html
fast/harness/user-preferred-language.html
inspector/debugger/setBreakpoint-autoContinue.html
Comment 27 Build Bot 2016-10-23 21:26:06 PDT
Created attachment 292577 [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 28 Build Bot 2016-10-23 21:31:35 PDT
Comment on attachment 292573 [details]
fix another crash

Attachment 292573 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2355020

New failing tests:
workers/bomb.html
Comment 29 Build Bot 2016-10-23 21:31:41 PDT
Created attachment 292578 [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
Comment 30 Build Bot 2016-10-23 23:36:56 PDT
Comment on attachment 292573 [details]
fix another crash

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

Number of test failures exceeded the failure limit.
Comment 31 Build Bot 2016-10-23 23:37:01 PDT
Created attachment 292586 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 32 Build Bot 2016-10-24 00:51:37 PDT
Comment on attachment 292573 [details]
fix another crash

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

New failing tests:
inspector/debugger/setBreakpoint-autoContinue.html
inspector/debugger/breakpoint-eval-with-exception.html
storage/indexeddb/database-wrapper.html
inspector/debugger/removeBreakpoint.html
inspector/debugger/pause-reason.html
storage/indexeddb/database-wrapper-private.html
Comment 33 Build Bot 2016-10-24 00:51:42 PDT
Created attachment 292593 [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 34 Filip Pizlo 2016-10-24 10:54:57 PDT
Created attachment 292627 [details]
it's getting more interesting

I fixed a gnarly deadlock when the mutator waits for the GC while the GC waits for the mutator, and a gnarly crash where the mutator can start a new DFG worklist after the GC had suspended the compiler threads.  Every day this patch gets more correct!
Comment 35 WebKit Commit Bot 2016-10-24 10:56:23 PDT
Attachment 292627 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1134:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 Filip Pizlo 2016-10-24 11:21:02 PDT
Created attachment 292631 [details]
fixing more bugs

Fixed a deadlock on VM shutdown.
Comment 37 WebKit Commit Bot 2016-10-24 11:25:28 PDT
Attachment 292631 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1134:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Filip Pizlo 2016-10-24 11:35:38 PDT
Created attachment 292634 [details]
more fixes
Comment 39 WebKit Commit Bot 2016-10-24 11:38:52 PDT
Attachment 292634 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1134:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Filip Pizlo 2016-10-24 12:05:43 PDT
Created attachment 292639 [details]
fixing more things
Comment 41 WebKit Commit Bot 2016-10-24 12:07:01 PDT
Attachment 292639 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1134:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Build Bot 2016-10-24 13:15:57 PDT
Comment on attachment 292639 [details]
fixing more things

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

Number of test failures exceeded the failure limit.
Comment 43 Build Bot 2016-10-24 13:16:03 PDT
Created attachment 292645 [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 44 Filip Pizlo 2016-10-24 13:26:41 PDT
Created attachment 292648 [details]
even more fixes
Comment 45 WebKit Commit Bot 2016-10-24 13:34:46 PDT
Attachment 292648 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1124:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1134:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Build Bot 2016-10-24 14:54:16 PDT
Comment on attachment 292648 [details]
even more fixes

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

Number of test failures exceeded the failure limit.
Comment 47 Build Bot 2016-10-24 14:54:23 PDT
Created attachment 292658 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 48 Build Bot 2016-10-24 15:18:23 PDT
Comment on attachment 292648 [details]
even more fixes

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

New failing tests:
workers/bomb.html
inspector/debugger/setBreakpoint-autoContinue.html
inspector/debugger/breakpoint-eval-with-exception.html
storage/indexeddb/database-wrapper.html
inspector/debugger/removeBreakpoint.html
fast/workers/dedicated-worker-lifecycle.html
inspector/debugger/pause-reason.html
storage/indexeddb/database-wrapper-private.html
Comment 49 Build Bot 2016-10-24 15:18:29 PDT
Created attachment 292660 [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 50 Build Bot 2016-10-24 15:21:55 PDT
Comment on attachment 292648 [details]
even more fixes

Attachment 292648 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2360762

New failing tests:
workers/bomb.html
Comment 51 Build Bot 2016-10-24 15:22:02 PDT
Created attachment 292662 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 52 Build Bot 2016-10-24 15:48:09 PDT
Comment on attachment 292648 [details]
even more fixes

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

New failing tests:
workers/bomb.html
storage/indexeddb/database-wrapper-private.html
inspector/debugger/breakpoint-eval-with-exception.html
storage/indexeddb/database-wrapper.html
inspector/debugger/removeBreakpoint.html
fast/workers/dedicated-worker-lifecycle.html
inspector/debugger/pause-reason.html
inspector/debugger/setBreakpoint-autoContinue.html
Comment 53 Build Bot 2016-10-24 15:48:16 PDT
Created attachment 292669 [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 54 Filip Pizlo 2016-10-24 18:00:20 PDT
Created attachment 292689 [details]
the most correct patch yet!
Comment 55 WebKit Commit Bot 2016-10-24 18:12:48 PDT
Attachment 292689 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1126:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1136:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 Filip Pizlo 2016-10-24 18:41:45 PDT
Created attachment 292698 [details]
fixed another deadlock
Comment 57 WebKit Commit Bot 2016-10-24 18:44:42 PDT
Attachment 292698 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1126:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1136:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 Build Bot 2016-10-24 20:03:31 PDT
Comment on attachment 292698 [details]
fixed another deadlock

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

Number of test failures exceeded the failure limit.
Comment 59 Build Bot 2016-10-24 20:03:38 PDT
Created attachment 292705 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 60 Build Bot 2016-10-24 20:16:19 PDT
Comment on attachment 292698 [details]
fixed another deadlock

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

New failing tests:
fast/workers/worker-messageport-gc.html
fast/workers/worker-document-leak.html
storage/indexeddb/database-wrapper-private.html
fast/workers/dedicated-worker-lifecycle.html
storage/indexeddb/database-wrapper.html
Comment 61 Build Bot 2016-10-24 20:16:26 PDT
Created attachment 292706 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 62 Build Bot 2016-10-24 20:21:28 PDT
Comment on attachment 292698 [details]
fixed another deadlock

Attachment 292698 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2362261

New failing tests:
fast/workers/worker-messageport-gc.html
fast/workers/dedicated-worker-lifecycle.html
Comment 63 Build Bot 2016-10-24 20:21:35 PDT
Created attachment 292708 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 64 Build Bot 2016-10-24 20:26:05 PDT
Comment on attachment 292698 [details]
fixed another deadlock

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

New failing tests:
fast/workers/worker-document-leak.html
storage/indexeddb/database-wrapper-private.html
inspector/page/searchInResources.html
storage/indexeddb/database-wrapper.html
fast/workers/dedicated-worker-lifecycle.html
fast/workers/worker-messageport-gc.html
Comment 65 Build Bot 2016-10-24 20:26:13 PDT
Created attachment 292709 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 66 Filip Pizlo 2016-10-24 21:45:09 PDT
I fixed another deadlock, this time caused by the worker runloop having heap access.

It's a real shame that if a thread blocks and its GC tries to stop the world, then this GC will have already suspended all of that thread's worklists.  This creates a pretty unhappy situation where a stop-the-world can cause deadlock if that thread is waiting for something that requires another VM to GC.  That other VM won't be able to GC because it won't be able to suspend the thread worklists, since only one GC can do that at a time.  What a shame.

Perhaps we should switch to having the compiler worklists get suspended after we have suspended the mutator.

Or we should have separate compiler worklists per VM.

Or we should have rightToRun per compiler plan, not per worklist.

Or switch to a more proper stop-the-world mechanism where there are multiple thread states and each thread can either be stopped or not.  A compiler plan would behave like a GC thread state.
Comment 67 Filip Pizlo 2016-10-24 21:49:50 PDT
It's pretty clear that from the standpoint of the Heap's stop-the-world machinery, a Plan is like a thread.  This is quite convenient because for STW we don't need a very heavy concept of threads.  We just need to be able to enumerate overall of them and have each of them track what I'm currently calling worldState.  It'll just become the thread state now.

When the Heap vends new ThreadState objects, it will take into account whether stop-the-world has begun.  If it has, then it will stop the world instead of trying to vend another one.  That will require some interesting locking protocol but I'm sure it's doable.  Probably we need the newThreadState algorithm to self-stop the thread - essentially, help the collector mark the current thread as shouldStop.  Or maybe we can have the current thread release heap access while creating a new thread state, and have creating a new thread state wait on the GC's stopTheWorld lock.  The GC could lock this lock before starting to stop the world.

Oh boy, this will be fun.
Comment 68 Filip Pizlo 2016-10-24 22:14:39 PDT
This doesn't totally fix our problems though.  In this scheme, a stalled GC will cause some compiler worklist thread to get stuck:

1) Thread waits for some event without releasing heap access.
2) GC starts signals stop the world to that thread's VM and its plans.
3) The DFG worklist thread working on that thread's VM's plan stops to wait for the GC.
4) The GC stalls waiting for the thread to stop, because that thread is waiting with heap access.
5) Other threads' VM's DFG plans stall because the DFG worklist thread is stalled.

It's starting to feel wrong that we have shared DFG worklists.

On the other hand, if we also say that the ThreadState corresponding to the main thread must be stopped before we request anyone else to stop, then the worst case is that some DFG worklist thread is stopped waiting for a GC to finish.  That's not as bad, but still quite yucky.  This is a bit like having a rightToRun in each Plan.

Another incremental improvement is to have the worklist decline to run a Plan if it cannot immediately get its rightToRun.  The problem is that there's no clear story for how the worklist would go about waiting for one out of many plans to become available to run.  This does not sound like a fun scheduling problem at all.

Maybe what we really need is instead of suspending compiler worklists, we want to remove all plans for the VM that wants the suspension.  This may mean waiting for a plan to finish.  Boy, this sure seems like rightToRun in each Plan.

The full solution would be:

- No more worklist suspension.
- After stopping the mutator, remove all of the mutator's plans from the worklists.  First lock the Plan and then remove it.  If it's currently running, we will block trying to lock it.  If it stops at a safepoint, then hang onto it.
- As an optimization, we could also remove plans from the worklist before stopping the mutator, if we can get their rightToRun locks quickly.

Alternatively, we could have each Worklist track a set of blocked VMs and it could skip plans from blocked VMs.  That seems like more complicated and less efficient.  Probably better to have the Heap stash the Plans from the plan queue and then restore them when the Heap is done.
Comment 69 Filip Pizlo 2016-10-25 08:39:31 PDT
OK, so it seems that I should stage this as follows:

1) Suspend the compiler worklists after suspending the mutator.
2) Make the GC enqueue a stopIfNecessary task onto the main thread's runloop.
Comment 70 Filip Pizlo 2016-10-25 09:58:12 PDT
(In reply to comment #69)
> OK, so it seems that I should stage this as follows:
> 
> 1) Suspend the compiler worklists after suspending the mutator.

This seems to just work.  I'm still testing though.

> 2) Make the GC enqueue a stopIfNecessary task onto the main thread's runloop.

I need to first make HeapTimer less crazy: https://bugs.webkit.org/show_bug.cgi?id=163947
Comment 71 Filip Pizlo 2016-10-25 11:15:29 PDT
Created attachment 292789 [details]
fix more things

I think that worker tests should not timeout anymore.

I fixed another incorrect debug assert.
Comment 72 WebKit Commit Bot 2016-10-25 12:39:28 PDT
Attachment 292789 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1021:  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:1126:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1136:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 73 Filip Pizlo 2016-10-25 13:23:00 PDT
I'm still seeing timeouts in indexeddb tests.  I think it's because the GC isn't running finalizers.  I'll work on that once bug 163947 is taken care of.
Comment 74 Build Bot 2016-10-25 14:44:12 PDT
Comment on attachment 292789 [details]
fix more things

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

Number of test failures exceeded the failure limit.
Comment 75 Build Bot 2016-10-25 14:44:18 PDT
Created attachment 292824 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 76 Build Bot 2016-10-25 15:08:06 PDT
Comment on attachment 292789 [details]
fix more things

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

New failing tests:
storage/indexeddb/database-wrapper-private.html
storage/indexeddb/database-wrapper.html
Comment 77 Build Bot 2016-10-25 15:08:12 PDT
Created attachment 292831 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 78 Build Bot 2016-10-25 15:25:03 PDT
Comment on attachment 292789 [details]
fix more things

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

New failing tests:
fast/harness/page-cache-crash-on-data-urls.html
storage/indexeddb/database-wrapper-private.html
inspector/sampling-profiler/call-frame-with-dom-functions.html
storage/indexeddb/database-wrapper.html
Comment 79 Build Bot 2016-10-25 15:25:09 PDT
Created attachment 292833 [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 80 Filip Pizlo 2016-10-25 17:00:43 PDT
Created attachment 292850 [details]
starting to make the runloop hacks work
Comment 81 Filip Pizlo 2016-10-25 22:35:07 PDT
Created attachment 292875 [details]
HeapTimer hack compiles
Comment 82 WebKit Commit Bot 2016-10-26 00:55:06 PDT
Attachment 292875 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1022:  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:1127:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1137:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 83 Build Bot 2016-10-26 02:14:51 PDT
Comment on attachment 292875 [details]
HeapTimer hack compiles

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

New failing tests:
storage/indexeddb/database-wrapper-private.html
storage/indexeddb/database-wrapper.html
Comment 84 Build Bot 2016-10-26 02:14:57 PDT
Created attachment 292906 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 85 Build Bot 2016-10-26 02:16:47 PDT
Comment on attachment 292875 [details]
HeapTimer hack compiles

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

Number of test failures exceeded the failure limit.
Comment 86 Build Bot 2016-10-26 02:16:53 PDT
Created attachment 292907 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 87 Build Bot 2016-10-26 02:22:57 PDT
Comment on attachment 292875 [details]
HeapTimer hack compiles

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

New failing tests:
storage/indexeddb/database-wrapper-private.html
storage/indexeddb/database-wrapper.html
Comment 88 Build Bot 2016-10-26 02:23:03 PDT
Created attachment 292908 [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 89 Filip Pizlo 2016-10-26 18:12:08 PDT
I know why we are failing the indexeddb tests.

We evaluate weak references in the GC thread.  IDBDatabase does this in a function that is called from the thing that evaluates whether the IDBDatabase is "live":

    bool result = hasEventListeners(eventNames().abortEvent) || hasEventListeners(eventNames().errorEvent) || hasEventListeners(eventNames().versionchangeEvent);

But eventNames() is a thread-local, and it will produce different things in the GC thread than on the main thread.

Ouch!

Three possible solutions:

1) Spoof thread-locals in the GC thread to make them return what the main thread would have returned.  We could do this globally (as a hack in ThreadSpecific) or on a case-by-case basis (as a hack at the use site).

2) Make this code get its eventNames() from somewhere else that isn't a thread-local.

3) Run the weak reference thing on the main thread.

I'm going to have to audit the code to see where else this comes up.  So far it seems that this is very specific to IndexedDB, which implies that we should just have a way of getting the eventNames() for the main thread.
Comment 90 Filip Pizlo 2016-10-26 18:30:17 PDT
I think that I want a feature in ThreadSpecific that detects when we're in the collector thread and asserts.

Some ThreadSpecifics will specifically call out that they are happy to be called from any thread, like the ones in WTF for ParkingLot and WordLock.  By default all others should barf.  This should shake out other similar bugs.
Comment 91 Filip Pizlo 2016-10-26 21:25:53 PDT
Created attachment 292989 [details]
with ThreadSpecific fixes

This fixes one of the indexeddb tests.  I think it might fix the other one, too.
Comment 92 Filip Pizlo 2016-10-27 07:08:11 PDT
Created attachment 293016 [details]
rebased patch
Comment 93 WebKit Commit Bot 2016-10-27 07:09:46 PDT
Attachment 293016 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1028:  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:1133:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1143:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 94 Build Bot 2016-10-27 08:31:08 PDT
Comment on attachment 293016 [details]
rebased patch

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

New failing tests:
svg/wicd/test-rightsizing-b.xhtml
Comment 95 Build Bot 2016-10-27 08:31:15 PDT
Created attachment 293020 [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 96 Build Bot 2016-10-27 08:39:57 PDT
Comment on attachment 293016 [details]
rebased patch

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

New failing tests:
http/tests/contentdispositionattachmentsandbox/referer-header-stripped.html
http/tests/contentdispositionattachmentsandbox/referer-header-stripped-with-meta-referer-never.html
Comment 97 Build Bot 2016-10-27 08:40:04 PDT
Created attachment 293021 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 98 Filip Pizlo 2016-10-27 09:43:55 PDT
Created attachment 293027 [details]
try to fix ios build
Comment 99 Geoffrey Garen 2016-10-27 10:37:15 PDT
> I'm going to have to audit the code to see where else this comes up.  So far
> it seems that this is very specific to IndexedDB, which implies that we
> should just have a way of getting the eventNames() for the main thread.

Atomic strings and Identifiers also depend on thread-specific string tables.

I think it's practical to establish a new rule that weak pointer hooks must be runnable on any thread -- but perhaps it's an easier starting point to say that they all run on the main thread. That would also solve the heap access lock concept in a straight-forward way.

Basically, I'm proposing an incremental development plan:

(1) Check in concurrent GC with a "finish GC" function that dispatches to the main / VM thread and does the final stack scan and weak reference scan.

(2) Add a heap access lock and enable weak reference scanning on secondary threads.

(3) Enable parallel weak reference scanning by divvying up the list of weak pointers to multiple threads.
Comment 100 WebKit Commit Bot 2016-10-27 11:34:32 PDT
Attachment 293027 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1028:  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:1133:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1143:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 101 Filip Pizlo 2016-10-27 12:15:32 PDT
(In reply to comment #99)
> > I'm going to have to audit the code to see where else this comes up.  So far
> > it seems that this is very specific to IndexedDB, which implies that we
> > should just have a way of getting the eventNames() for the main thread.
> 
> Atomic strings and Identifiers also depend on thread-specific string tables.

My current patch, which passes all WK2 release tests and all JSC stress tests (but still has some failures in debug and WK1 release), already implements my disallow-on-GC-thread ThreadSpecific, and the atomic string table and Identifier stuff already uses it.  So, we already know that the GC thread never touches them.

> 
> I think it's practical to establish a new rule that weak pointer hooks must
> be runnable on any thread -- but perhaps it's an easier starting point to
> say that they all run on the main thread. 

The isReachableFromOpaque weak pointer hooks are already runnable on any thread in this patch.

> That would also solve the heap
> access lock concept in a straight-forward way.

You need a heap access lock for any concurrent GC because there will always be multiple stop-the-world increments that require reaching a safepoint.  The heap access lock implements safepoints.

Currently the patch schedules the WebCore-observable parts of marking activity like this:

Destroying weak handles still happens on the main thread.  This is where atomic string and Identifier stuff will happen because of destructors.  None of this code has to change since it stays on the main thread.

Visiting children already happens on many threads, and this already works.  None of this code has to change.

reachableFromOpaqueRoots will now be called from the collector thread, but while the world is stopped.  This appears to already work in this patch, and it runs on the GC thread, and we don't seem to ever hit the ThreadSpecific assertion.  Only the WordLock, ParkingLot, mayBeGCThread, and mainThread ThreadSpecifics are safe to use from GC threads.  All others will RELEASE_ASSERT now if they are and this Just Works.

> 
> Basically, I'm proposing an incremental development plan:
> 
> (1) Check in concurrent GC with a "finish GC" function that dispatches to
> the main / VM thread and does the final stack scan and weak reference scan.

I'm not sure that there is really such a thing as a "final" stack scan or weak reference scan, since this will be a fixpoint.  We could potentially discover a lot of things and have a lot of subsequent GC activity after one iteration of the fixpoint.  So, if this is really a "finishGC()" in the sense that it really finishes all of the GC cycle, then this is a significantly different design than what we had been talking about previously.  In any case, this patch already runs weak reference scanning on the GC thread.

> 
> (2) Add a heap access lock and enable weak reference scanning on secondary
> threads.

Are you suggesting that the concurrent GC with a finishGC() at the end doesn't need a heap access lock?  I don't see how that's possible.  The concurrent GC has to have a way to wait for the mutator to reach a safepoint before it starts GC.  So, even in a GC that does none of the fixpoint concurrently, you would still have two stop-the-world increments in the collection: (1) a stop-the-world safepoint just to make sure that the mutator knows that concurrent marking has begun and (2) a stop-the-world safepoint at the end to finishGC().  Since this involves safepoints, it involves a heap access lock.  

What you're proposing isn't really simpler than this patch.  I would probably make your proposed algorithm work by moving more stuff out of collectInThread() and into finalize(). I believe that they only thing you'd get in return for this is that you'd be able to remove my super-simple EventNames fix from indexeddb.  On the other hand, I'd have to un-delete the main thread special case from ConservativeRoots, so even if the patch is a tiny bit smaller, the resulting codebase isn't any smaller.  Doesn't seem worth it.

Notably, you'd still need these things:

- You'd still want the ThreadSpecific "can use on GC thread" assertion to ensure that nothing else in collectInThread() accesses atomic string tables, event name tables, etc.

- You'd still want the heap access lock, because that's how you stop the world.

> 
> (3) Enable parallel weak reference scanning by divvying up the list of weak
> pointers to multiple threads.

This patch doesn't enable parallel or concurrent weak reference scanning, and that is not on the roadmap.  This only does weak reference scanning in the collector thread with the world stopped, since doing it on the main thread would mean that the collector would have to bounce more work between the two threads and it's just more awkward to write it that way.  It's much better if all of the work is simply on the collector thread, and the only thing that seems genuinely unhappy about this is indexeddb and I already fixed that bug.
Comment 102 Filip Pizlo 2016-10-27 12:16:24 PDT
Since this seems to be passing enough stuff to suggest that it's probably the right direction, I'm going to take a break from correctness to run some benchmarks.  I haven't done that at all yet.
Comment 103 Filip Pizlo 2016-10-27 13:02:48 PDT
Perf is great so far:

Benchmark report for SunSpider, LongSpider, V8Spider, Octane, Kraken, and AsmBench on murderface (MacBookPro11,5).

VMs tested:
"TipOfTree" at /Volumes/Data/secondary/OpenSource/WebKitBuild/Release/jsc (r207989)
"Things" at /Volumes/Data/quartary/OpenSource/WebKitBuild/Release/jsc (r207989)

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to
get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                                                TipOfTree                   Things                                      
SunSpider:
   3d-cube                                    4.6527+-0.1878     ?      4.6617+-0.1189        ?
   3d-morph                                   4.7100+-0.0686            4.6710+-0.0325        
   3d-raytrace                                4.7902+-0.1082     ?      4.8546+-0.1238        ? might be 1.0134x slower
   access-binary-trees                        1.9347+-0.0490     ?      1.9559+-0.1004        ? might be 1.0109x slower
   access-fannkuch                            4.8070+-0.2231            4.7230+-0.1193          might be 1.0178x faster
   access-nbody                               2.4766+-0.2001     ?      2.5084+-0.2280        ? might be 1.0128x slower
   access-nsieve                              2.8875+-0.0528            2.8809+-0.0430        
   bitops-3bit-bits-in-byte                   1.0735+-0.0290     ?      1.1021+-0.0249        ? might be 1.0266x slower
   bitops-bits-in-byte                        2.6907+-0.0376     ?      2.7250+-0.0480        ? might be 1.0127x slower
   bitops-bitwise-and                         2.1425+-0.3722            2.0067+-0.0695          might be 1.0676x faster
   bitops-nsieve-bits                         3.0332+-0.0367     ?      3.0350+-0.0569        ?
   controlflow-recursive                      2.2744+-0.0609            2.2002+-0.0293          might be 1.0337x faster
   crypto-aes                                 4.4504+-0.3182            4.4288+-0.1987        
   crypto-md5                                 2.7305+-0.1434            2.6256+-0.0479          might be 1.0400x faster
   crypto-sha1                                2.7208+-0.1039     ?      2.7257+-0.2000        ?
   date-format-tofte                          6.7618+-0.1738     ?      6.8083+-0.2957        ?
   date-format-xparb                          4.5375+-0.1592     ?      4.5996+-0.1013        ? might be 1.0137x slower
   math-cordic                                2.6473+-0.0375     ?      2.6947+-0.0347        ? might be 1.0179x slower
   math-partial-sums                          3.9147+-0.1330     ?      3.9814+-0.1427        ? might be 1.0170x slower
   math-spectral-norm                         2.0038+-0.0481            1.9518+-0.0347          might be 1.0267x faster
   regexp-dna                                 6.4300+-0.4733            6.1317+-0.0848          might be 1.0487x faster
   string-base64                              4.5808+-0.1054     ?      4.6206+-0.2944        ?
   string-fasta                               5.4294+-0.0961     ?      5.4857+-0.2441        ? might be 1.0104x slower
   string-tagcloud                            8.2103+-0.1116     ?      8.2781+-0.0947        ?
   string-unpack-code                        19.4798+-0.8275           18.9618+-0.5403          might be 1.0273x faster
   string-validate-input                      4.2388+-0.0656     ?      4.2717+-0.1933        ?

   <arithmetic>                               4.4465+-0.0591            4.4188+-0.0284          might be 1.0063x faster

                                                TipOfTree                   Things                                      
LongSpider:
   3d-cube                                  776.4021+-5.2870     !    791.2607+-7.6624        ! definitely 1.0191x slower
   3d-morph                                 563.8688+-2.3972     ?    567.4722+-4.6106        ?
   3d-raytrace                              451.1426+-3.2141     ?    452.4367+-3.8279        ?
   access-binary-trees                      779.7903+-6.8410     ?    786.5005+-5.9033        ?
   access-fannkuch                          237.3684+-7.1158     ?    240.2315+-7.7376        ? might be 1.0121x slower
   access-nbody                             505.1886+-4.2990          503.8432+-3.1403        
   access-nsieve                            278.8836+-3.7132     !    288.0982+-4.0555        ! definitely 1.0330x slower
   bitops-3bit-bits-in-byte                  31.8845+-0.7088     ?     32.2106+-0.6041        ? might be 1.0102x slower
   bitops-bits-in-byte                       84.7680+-2.7632           83.1273+-2.7359          might be 1.0197x faster
   bitops-nsieve-bits                       369.4695+-4.8112          369.0998+-3.5582        
   controlflow-recursive                    395.2949+-2.9499     !    408.1899+-2.9458        ! definitely 1.0326x slower
   crypto-aes                               530.1221+-5.6100     ?    538.3452+-3.7746        ? might be 1.0155x slower
   crypto-md5                               446.9192+-5.6517     ?    451.4897+-4.4212        ? might be 1.0102x slower
   crypto-sha1                              596.8916+-6.5691          595.3906+-5.8322        
   date-format-tofte                        337.2380+-4.7041          332.8972+-4.9211          might be 1.0130x faster
   date-format-xparb                        625.6261+-18.3742         610.8234+-4.9431          might be 1.0242x faster
   hash-map                                 136.9181+-2.2807          134.5576+-2.0029          might be 1.0175x faster
   math-cordic                              403.1510+-5.0021          402.6423+-2.4192        
   math-partial-sums                        284.3478+-2.5260     ?    285.1679+-2.5942        ?
   math-spectral-norm                       517.4135+-3.1750          517.1621+-4.3033        
   string-base64                            467.4638+-4.4233     ^    460.1631+-2.5808        ^ definitely 1.0159x faster
   string-fasta                             329.9721+-4.0270     ?    334.8429+-3.4549        ? might be 1.0148x slower
   string-tagcloud                          166.7167+-2.6236          166.1812+-2.0011        

   <geometric>                              335.8274+-0.8935     ?    336.8070+-0.8834        ? might be 1.0029x slower

                                                TipOfTree                   Things                                      
V8Spider:
   crypto                                    34.2478+-0.4391           34.2182+-0.3229        
   deltablue                                 48.1972+-0.8972     ?     48.2258+-1.4338        ?
   earley-boyer                              34.1408+-0.3356     ?     34.3336+-0.4549        ?
   raytrace                                  19.0341+-0.4391     ?     19.1110+-0.5368        ?
   regexp                                    50.7595+-0.9198     ?     50.8155+-0.6694        ?
   richards                                  38.7273+-0.9237           38.6586+-0.9929        
   splay                                     28.4917+-0.7421           28.4896+-0.6241        

   <geometric>                               34.6522+-0.4386     ?     34.6913+-0.3689        ? might be 1.0011x slower

                                                TipOfTree                   Things                                      
Octane:
   encrypt                                   0.14857+-0.00072          0.14850+-0.00078       
   decrypt                                   2.57234+-0.02606    ?     2.57418+-0.01773       ?
   deltablue                        x2       0.11815+-0.00053          0.11753+-0.00161       
   earley                                    0.23716+-0.00100    ?     0.23924+-0.00141       ?
   boyer                                     3.98388+-0.02381    ?     3.99924+-0.02691       ?
   navier-stokes                    x2       4.60966+-0.02582          4.60773+-0.02698       
   raytrace                         x2       0.65536+-0.00597    ?     0.66073+-0.00348       ?
   richards                         x2       0.07974+-0.00095          0.07927+-0.00058       
   splay                            x2       0.31384+-0.00362          0.31017+-0.00163         might be 1.0118x faster
   regexp                           x2      16.63018+-0.54771    ?    16.99564+-0.46518       ? might be 1.0220x slower
   pdfjs                            x2      38.74467+-0.40520    ?    39.19560+-0.43265       ? might be 1.0116x slower
   mandreel                         x2      40.38550+-0.21926         40.18469+-0.31253       
   gbemu                            x2      29.04846+-1.22493         29.02234+-0.21386       
   closure                                   0.47282+-0.00344    ?     0.47365+-0.00371       ?
   jquery                                    6.49918+-0.04669          6.47977+-0.02798       
   box2d                            x2       8.80203+-0.06882    ?     8.82370+-0.06239       ?
   zlib                             x2     331.70140+-8.70431    ?   336.82563+-5.32340       ? might be 1.0154x slower
   typescript                       x2     600.55415+-10.34598   ?   607.84851+-6.04161       ? might be 1.0121x slower

   <geometric>                               4.70927+-0.02325    ?     4.72502+-0.01604       ? might be 1.0033x slower

                                                TipOfTree                   Things                                      
Kraken:
   ai-astar                                   91.113+-1.745             89.886+-1.267           might be 1.0136x faster
   audio-beat-detection                       35.556+-0.296             35.453+-0.191         
   audio-dft                                  93.895+-1.278             93.536+-1.923         
   audio-fft                                  27.521+-0.317      ?      27.819+-0.873         ? might be 1.0108x slower
   audio-oscillator                           42.688+-0.190      ?      43.585+-1.084         ? might be 1.0210x slower
   imaging-darkroom                           56.379+-0.693             56.351+-0.345         
   imaging-desaturate                         43.639+-2.340             41.814+-0.820           might be 1.0437x faster
   imaging-gaussian-blur                      59.634+-2.096             57.450+-2.754           might be 1.0380x faster
   json-parse-financial                       32.504+-0.581      ?      32.663+-0.942         ?
   json-stringify-tinderbox                   22.383+-0.709             22.347+-1.097         
   stanford-crypto-aes                        34.432+-0.288      ?      35.335+-1.387         ? might be 1.0262x slower
   stanford-crypto-ccm                        31.664+-1.701      ?      32.554+-1.874         ? might be 1.0281x slower
   stanford-crypto-pbkdf2                     89.871+-1.205             89.863+-1.449         
   stanford-crypto-sha256-iterative           29.759+-0.997             29.385+-0.232           might be 1.0127x faster

   <arithmetic>                               49.360+-0.391             49.146+-0.352           might be 1.0044x faster

                                                TipOfTree                   Things                                      
AsmBench:
   bigfib.cpp                               408.4686+-2.9424     ?    410.1206+-2.4365        ?
   cray.c                                   356.6385+-3.0432     ?    359.9752+-2.8078        ?
   dry.c                                    422.1167+-56.5409         395.0232+-4.1306          might be 1.0686x faster
   FloatMM.c                                612.7571+-2.6223     ?    624.3109+-19.7745       ? might be 1.0189x slower
   gcc-loops.cpp                           3374.3796+-21.8156    ?   3390.4244+-18.3681       ?
   n-body.c                                 755.0362+-5.0151          754.5440+-3.9216        
   Quicksort.c                              375.6331+-4.2070          371.6725+-4.4562          might be 1.0107x faster
   stepanov_container.cpp                  3203.4247+-64.4180    ?   3209.5418+-51.7800       ?
   Towers.c                                 224.7176+-2.1099     ?    224.9224+-2.9884        ?

   <geometric>                              665.6664+-9.3794          663.6035+-3.2991          might be 1.0031x faster

                                                TipOfTree                   Things                                      
Geomean of preferred means:
   <scaled-result>                           44.7236+-0.0956           44.6792+-0.1312          might be 1.0010x faster

I will run browser benchmarks next.
Comment 104 Build Bot 2016-10-27 13:04:00 PDT
Comment on attachment 293027 [details]
try to fix ios build

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

New failing tests:
fast/harness/use-page-cache.html
Comment 105 Build Bot 2016-10-27 13:04:07 PDT
Created attachment 293046 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 106 Geoffrey Garen 2016-10-27 14:18:31 PDT
Most of the EWS crashes look like atomic string reference counting bugs:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001077910b7 WTFCrash + 39
1   com.apple.JavaScriptCore      	0x000000010779516b WTF::AtomicStringImpl::remove(WTF::AtomicStringImpl*) + 267
2   com.apple.JavaScriptCore      	0x00000001077e5ad3 WTF::StringImpl::~StringImpl() + 163
3   com.apple.JavaScriptCore      	0x00000001077e5c45 WTF::StringImpl::~StringImpl() + 21
4   com.apple.JavaScriptCore      	0x00000001077e5c65 WTF::StringImpl::destroy(WTF::StringImpl*) + 21
5   com.apple.JavaScriptCore      	0x00000001066587b4 WTF::StringImpl::deref() + 52
6   com.apple.JavaScriptCore      	0x0000000107470ed1 JSC::SourceProviderCacheItem::~SourceProviderCacheItem() + 65

Two look like GC bugs:

Thread 21 Crashed:: WTF::AutomaticThread
0   com.apple.JavaScriptCore      	0x000000010dff70b7 WTFCrash + 39
1   com.apple.JavaScriptCore      	0x000000010dff70d9 WTFCrashWithSecurityImplication + 9
2   com.apple.JavaScriptCore      	0x000000010ce34a5b JSC::StructureIDTable::get(unsigned int) + 91
3   com.apple.JavaScriptCore      	0x000000010cebec45 JSC::JSCell::structure() const + 53
4   com.apple.JavaScriptCore      	0x000000010ddf5d02 JSC::SlotVisitor::appendJSCellOrAuxiliary(JSC::HeapCell*) + 210
5   com.apple.JavaScriptCore      	0x000000010ddf5c10 JSC::SlotVisitor::append(JSC::ConservativeRoots&) + 96

Thread 35 Crashed:: WTF::AutomaticThread
0   com.apple.JavaScriptCore      	0x000000010c69d0b7 WTFCrash + 39
1   com.apple.JavaScriptCore      	0x000000010bcc07a9 JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNullOrUndefined(JSC::DFG::Edge) + 137
2   com.apple.JavaScriptCore      	0x000000010bc5944b JSC::DFG::SpeculativeJIT::compare(JSC::DFG::Node*, JSC::MacroAssemblerX86Common::RelationalCondition, JSC::MacroAssemblerX86Common::DoubleCondition, unsigned long (*)(JSC::ExecState*, long long, long long)) + 939
3   com.apple.JavaScriptCore      	0x000000010bccda58 JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*) + 8664
4   com.apple.JavaScriptCore      	0x000000010bc469dc JSC::DFG::SpeculativeJIT::compileCurrentBlock() + 1628
5   com.apple.JavaScriptCore      	0x000000010bc471b6 JSC::DFG::SpeculativeJIT::compile() + 182
Comment 107 Filip Pizlo 2016-10-27 14:47:50 PDT
(In reply to comment #106)
> Most of the EWS crashes look like atomic string reference counting bugs:
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x00000001077910b7 WTFCrash + 39
> 1   com.apple.JavaScriptCore      	0x000000010779516b
> WTF::AtomicStringImpl::remove(WTF::AtomicStringImpl*) + 267
> 2   com.apple.JavaScriptCore      	0x00000001077e5ad3
> WTF::StringImpl::~StringImpl() + 163
> 3   com.apple.JavaScriptCore      	0x00000001077e5c45
> WTF::StringImpl::~StringImpl() + 21
> 4   com.apple.JavaScriptCore      	0x00000001077e5c65
> WTF::StringImpl::destroy(WTF::StringImpl*) + 21
> 5   com.apple.JavaScriptCore      	0x00000001066587b4
> WTF::StringImpl::deref() + 52
> 6   com.apple.JavaScriptCore      	0x0000000107470ed1
> JSC::SourceProviderCacheItem::~SourceProviderCacheItem() + 65
> 
> Two look like GC bugs:
> 
> Thread 21 Crashed:: WTF::AutomaticThread
> 0   com.apple.JavaScriptCore      	0x000000010dff70b7 WTFCrash + 39
> 1   com.apple.JavaScriptCore      	0x000000010dff70d9
> WTFCrashWithSecurityImplication + 9
> 2   com.apple.JavaScriptCore      	0x000000010ce34a5b
> JSC::StructureIDTable::get(unsigned int) + 91
> 3   com.apple.JavaScriptCore      	0x000000010cebec45
> JSC::JSCell::structure() const + 53
> 4   com.apple.JavaScriptCore      	0x000000010ddf5d02
> JSC::SlotVisitor::appendJSCellOrAuxiliary(JSC::HeapCell*) + 210
> 5   com.apple.JavaScriptCore      	0x000000010ddf5c10
> JSC::SlotVisitor::append(JSC::ConservativeRoots&) + 96
> 
> Thread 35 Crashed:: WTF::AutomaticThread
> 0   com.apple.JavaScriptCore      	0x000000010c69d0b7 WTFCrash + 39
> 1   com.apple.JavaScriptCore      	0x000000010bcc07a9
> JSC::DFG::SpeculativeJIT::
> nonSpeculativeNonPeepholeCompareNullOrUndefined(JSC::DFG::Edge) + 137
> 2   com.apple.JavaScriptCore      	0x000000010bc5944b
> JSC::DFG::SpeculativeJIT::compare(JSC::DFG::Node*,
> JSC::MacroAssemblerX86Common::RelationalCondition,
> JSC::MacroAssemblerX86Common::DoubleCondition, unsigned long
> (*)(JSC::ExecState*, long long, long long)) + 939
> 3   com.apple.JavaScriptCore      	0x000000010bccda58
> JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*) + 8664
> 4   com.apple.JavaScriptCore      	0x000000010bc469dc
> JSC::DFG::SpeculativeJIT::compileCurrentBlock() + 1628
> 5   com.apple.JavaScriptCore      	0x000000010bc471b6
> JSC::DFG::SpeculativeJIT::compile() + 182

Sure, it still has bugs. I'm fixing them. But right now I'm taking a diversion to look at perf and check stability on perf tests. So far it looks like there may be a PLT regression, in which case I sort of want to tackle it head-on with the more ambitious version of this patch, because it gives me more wiggle room.
Comment 108 Filip Pizlo 2016-10-27 15:04:24 PDT
Let's make are we aren't talking past each other. I believe there will be a heap access lock no matter what, and that part of the patch now seems pretty solid. Neither this patch or any patch I'm planning will make weak reference processing concurrent. It happens with the world stopped. So the question is: should a stop-the-world activity run on the collector thread or the main thread?  The only difference is that running on the collector thread is easier to write (you don't need a callback or other dispatch mechanism for telling the main thread what to do) but it has different thread specifics. I thought it would be a shame to engineer a dispatch-to-main-thread mechanism to protect against thread specifics without knowing if this is really a problem. So I added the assertion in ThreadSpecific and found that it only fired for the IndexedDB bug that I fixed. So to me, the trade-off is a no-brained. It's highly unlikely that the remaining bugs are related to thread specifics because they will trap when accessed on the GC thread. 

I hope this makes sense. Since this patch runs all of this stuff with the world stopped, and since we need a heap access lock anyway to make sure that the mutator and collector can wait on each other (which they will need in any variant of a concurrent collector), the only difference is what happens to thread locals, and we currently have no evidence that the thread locals are accessed from the GC thread.
Comment 109 Geoffrey Garen 2016-10-27 15:24:31 PDT
Not trying to argue with your approach here -- just summarizing the failures.
Comment 110 Filip Pizlo 2016-10-27 15:33:48 PDT
(In reply to comment #109)
> Not trying to argue with your approach here -- just summarizing the failures.

Oh, sorry!
Comment 111 Filip Pizlo 2016-10-27 15:42:15 PDT
PLT looks like a 0.8% regression with 99% confidence.

I think that before I think about that too much, I need to see what the perf is on other benchmarks.
Comment 112 Filip Pizlo 2016-10-27 16:48:47 PDT
JetStream looks completely even.

I'll check Speedometer and JSBench.
Comment 113 Filip Pizlo 2016-10-27 16:49:13 PDT
(In reply to comment #112)
> JetStream looks completely even.
> 
> I'll check Speedometer and JSBench.

As in, it's 0.06% faster with 11% confidence. ;-)
Comment 114 Filip Pizlo 2016-10-27 17:49:30 PDT
Speedometer is 0.03% faster with 5.8% confidence.  (That means neutral.)
Comment 115 Filip Pizlo 2016-10-27 18:06:34 PDT
JSBench is even-steven.

This calls into question the PLT result, because:

- I've had two GC patches now with false PLT results on this machine.  Most recently it was a regression, but then when I landed the patch it was neutral on all bots; before that it was a progression that ended up neutral on the bots.

- I changed the GC, and GC tests are neutral.  Looking specifically at JetStream/splay and earley-boyer those are neutral.

- JSBench is usually correlated with PLT on JSC regressions, and here JSBench is claiming neutrality.

I'll get back to fixing the remaining crashes.
Comment 116 Filip Pizlo 2016-10-30 11:03:00 PDT
Created attachment 293357 [details]
rebased
Comment 117 WebKit Commit Bot 2016-10-30 11:06:03 PDT
Attachment 293357 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1028:  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:1133:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1143:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 118 Build Bot 2016-10-30 12:37:45 PDT
Comment on attachment 293357 [details]
rebased

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

New failing tests:
fast/harness/use-page-cache.html
http/tests/contentdispositionattachmentsandbox/referer-header-stripped-with-meta-referer-no-referrer.html
http/tests/contentdispositionattachmentsandbox/referer-header-stripped-with-meta-referer-always.html
http/tests/contentdispositionattachmentsandbox/referer-header-stripped-with-meta-referer-unsafe-url.html
http/tests/contentdispositionattachmentsandbox/referer-header-stripped-with-meta-referer-never.html
Comment 119 Build Bot 2016-10-30 12:37:52 PDT
Created attachment 293365 [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 120 Build Bot 2016-10-30 17:46:49 PDT
Comment on attachment 293357 [details]
rebased

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

New failing tests:
fast/harness/internals-object.html
Comment 121 Build Bot 2016-10-30 17:46:56 PDT
Created attachment 293376 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 122 Filip Pizlo 2016-11-01 21:18:07 PDT
Looks like at least some of the crashes are because JSLock's call to acquireAccess comes before it does the atomic string table swap.  So, if acquireAccess calls finalize, then it will run some code with the wrong string table.

That was an easy fix.
Comment 123 Filip Pizlo 2016-11-01 21:18:43 PDT
Created attachment 293636 [details]
the patch
Comment 124 Filip Pizlo 2016-11-01 21:55:08 PDT
Created attachment 293639 [details]
the patch

Rebased.
Comment 125 Filip Pizlo 2016-11-01 21:56:28 PDT
I think that this latest patch still has a GC crash in it.  This will be fun...
Comment 126 WebKit Commit Bot 2016-11-01 21:57:26 PDT
Attachment 293639 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1028:  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:1133:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1143:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 127 Filip Pizlo 2016-11-02 07:41:27 PDT
Interesting, it seems that the bots think that the patch is as stable as trunk.

I saw a GC crash in debug layout tests.  I'll run debug layout tests again.  If I can't repro, then I think it's safe to treat that as a fluke.

I think this means that the patch is ready for review.
Comment 128 Filip Pizlo 2016-11-02 07:44:43 PDT
Created attachment 293660 [details]
the patch

Updated the changelog.
Comment 129 WebKit Commit Bot 2016-11-02 07:48:39 PDT
Attachment 293660 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1028:  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:1133:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1143:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 130 Filip Pizlo 2016-11-02 08:02:03 PDT
Comment on attachment 293660 [details]
the patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:153
> +    if (m_isScheduled)
> +        return;

I will remove this.  It's not meant to be here.

> Source/JavaScriptCore/heap/HeapTimer.cpp:224
> +    if (m_isScheduled)
> +        return;

Ditto.
Comment 131 Filip Pizlo 2016-11-02 10:31:22 PDT
(In reply to comment #127)
> Interesting, it seems that the bots think that the patch is as stable as
> trunk.
> 
> I saw a GC crash in debug layout tests.  I'll run debug layout tests again. 
> If I can't repro, then I think it's safe to treat that as a fluke.
> 
> I think this means that the patch is ready for review.

I can no longer reproduce that GC crash, but I'll try again for giggles.
Comment 132 Geoffrey Garen 2016-11-02 14:03:18 PDT
Comment on attachment 293660 [details]
the patch

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

We go out of our way to track heap access precisely, but our main client, WebCore, just says "I always have heap access". Do we plan to take advantage of fine-grained heap access in the future?

> Source/JavaScriptCore/heap/Heap.cpp:1018
> +void Heap::collectAsync(Optional<CollectionScope> scope)

Is it meaningful to request a few collections in sequence, such as { Eden, Nullopt, Full }? I can't think of a use for this feature.

Since we're already coalescing repeat requests, and since Nullopt is a superset of Eden, and Full is a superset of Nullopt, this would be clearer to me as a single Optional<CollectionScope> that upgraded itself Eden => Nullopt => Full as necessary.

> Source/JavaScriptCore/heap/Heap.cpp:1195
> +    m_collectorBelievesThatTheMutatorIsStopped = true;

Should this assignment move into stopTheMutator()? Or do we consider compiler threads to be part of the mutator?

> Source/JavaScriptCore/heap/Heap.cpp:1369
> +bool Heap::handleDidJIT(unsigned oldState)

I think we want a more specific name here like "handleGCDidJIT".

> Source/JavaScriptCore/heap/Heap.h:597
> +    static const unsigned didJITBit = 1u << 3u; // Set when the GC did some JITing, so on resume we need to cpuid.

I think we want a more specific name here like "gcDidJITBit".

> Source/JavaScriptCore/heap/ReleaseHeapAccessScope.h:38
> +// possibly infinitely long-running) ensures that the GC can finish a collection cycle while you are

long-running) code
Comment 133 Filip Pizlo 2016-11-02 14:10:17 PDT
(In reply to comment #132)
> Comment on attachment 293660 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293660&action=review
> 
> We go out of our way to track heap access precisely, but our main client,
> WebCore, just says "I always have heap access". Do we plan to take advantage
> of fine-grained heap access in the future?

Workers and JS API clients take advantage of this already in this patch.

> 
> > Source/JavaScriptCore/heap/Heap.cpp:1018
> > +void Heap::collectAsync(Optional<CollectionScope> scope)
> 
> Is it meaningful to request a few collections in sequence, such as { Eden,
> Nullopt, Full }? I can't think of a use for this feature.

You can request a Full collection while an Eden collection is ongoing.

> 
> Since we're already coalescing repeat requests, and since Nullopt is a
> superset of Eden, and Full is a superset of Nullopt, this would be clearer
> to me as a single Optional<CollectionScope> that upgraded itself Eden =>
> Nullopt => Full as necessary.

The eden collection or the Nullopt collection could be happening right now.  It can't be upgraded while it's running.

> 
> > Source/JavaScriptCore/heap/Heap.cpp:1195
> > +    m_collectorBelievesThatTheMutatorIsStopped = true;
> 
> Should this assignment move into stopTheMutator()? Or do we consider
> compiler threads to be part of the mutator?

I renamed the field to collectorBelievesThatTheWorldIsStopped.

> 
> > Source/JavaScriptCore/heap/Heap.cpp:1369
> > +bool Heap::handleDidJIT(unsigned oldState)
> 
> I think we want a more specific name here like "handleGCDidJIT".

Renamed this method and the flag to gcDidJIT/setGCDidJIT/handleGCDidJIT.

> 
> > Source/JavaScriptCore/heap/Heap.h:597
> > +    static const unsigned didJITBit = 1u << 3u; // Set when the GC did some JITing, so on resume we need to cpuid.
> 
> I think we want a more specific name here like "gcDidJITBit".

Yup.

> 
> > Source/JavaScriptCore/heap/ReleaseHeapAccessScope.h:38
> > +// possibly infinitely long-running) ensures that the GC can finish a collection cycle while you are
> 
> long-running) code

Fixed.
Comment 134 Filip Pizlo 2016-11-02 14:56:33 PDT
Created attachment 293699 [details]
patch for landing
Comment 135 WebKit Commit Bot 2016-11-02 14:59:44 PDT
Attachment 293699 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1028:  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:1133:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:1143:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/ThreadSpecific.h:95:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 136 Filip Pizlo 2016-11-02 15:04:29 PDT
Landed in https://trac.webkit.org/changeset/208306
Comment 137 Chris Dumez 2016-11-08 12:41:57 PST
Seems to have caused crashes when running Speedometer:
rdar://problem/29079741
Comment 138 Filip Pizlo 2016-11-08 12:42:53 PST
(In reply to comment #137)
> Seems to have caused crashes when running Speedometer:
> rdar://problem/29079741

We think it's this: https://bugs.webkit.org/show_bug.cgi?id=164433