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.
(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.
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.
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.
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
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
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.
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.
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
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
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 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
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
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!
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.
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.
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.
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.
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
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.
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 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
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
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 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
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
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.
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.
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 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
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
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 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
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
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.
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.
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.
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.
(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
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.
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.
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
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
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
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.
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
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
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
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.
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.
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.
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
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
> 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.
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.
(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.
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.
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
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.
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.
(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. ;-)
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.
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.
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
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
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.
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.
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.
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.
(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 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
(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.
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.
2016-10-22 14:12 PDT, Filip Pizlo
2016-10-22 16:18 PDT, Filip Pizlo
2016-10-22 21:52 PDT, Filip Pizlo
2016-10-22 22:11 PDT, Filip Pizlo
2016-10-22 23:09 PDT, Filip Pizlo
2016-10-23 10:22 PDT, Filip Pizlo
2016-10-23 11:12 PDT, Filip Pizlo
2016-10-23 13:23 PDT, Filip Pizlo
2016-10-23 13:26 PDT, Filip Pizlo
2016-10-23 16:02 PDT, Filip Pizlo
2016-10-23 17:07 PDT, Build Bot
2016-10-23 17:19 PDT, Build Bot
2016-10-23 18:58 PDT, Filip Pizlo
2016-10-23 19:45 PDT, Filip Pizlo
2016-10-23 19:59 PDT, Filip Pizlo
2016-10-23 21:26 PDT, Build Bot
2016-10-23 21:31 PDT, Build Bot
2016-10-23 23:37 PDT, Build Bot
2016-10-24 00:51 PDT, Build Bot
2016-10-24 10:54 PDT, Filip Pizlo
2016-10-24 11:21 PDT, Filip Pizlo
2016-10-24 11:35 PDT, Filip Pizlo
2016-10-24 12:05 PDT, Filip Pizlo
2016-10-24 13:16 PDT, Build Bot
2016-10-24 13:26 PDT, Filip Pizlo
2016-10-24 14:54 PDT, Build Bot
2016-10-24 15:18 PDT, Build Bot
2016-10-24 15:22 PDT, Build Bot
2016-10-24 15:48 PDT, Build Bot
2016-10-24 18:00 PDT, Filip Pizlo
2016-10-24 18:41 PDT, Filip Pizlo
2016-10-24 20:03 PDT, Build Bot
2016-10-24 20:16 PDT, Build Bot
2016-10-24 20:21 PDT, Build Bot
2016-10-24 20:26 PDT, Build Bot
2016-10-25 11:15 PDT, Filip Pizlo
2016-10-25 14:44 PDT, Build Bot
2016-10-25 15:08 PDT, Build Bot
2016-10-25 15:25 PDT, Build Bot
2016-10-25 17:00 PDT, Filip Pizlo
2016-10-25 22:35 PDT, Filip Pizlo
2016-10-26 02:14 PDT, Build Bot
2016-10-26 02:16 PDT, Build Bot
2016-10-26 02:23 PDT, Build Bot
2016-10-26 21:25 PDT, Filip Pizlo
2016-10-27 07:08 PDT, Filip Pizlo
2016-10-27 08:31 PDT, Build Bot
2016-10-27 08:40 PDT, Build Bot
2016-10-27 09:43 PDT, Filip Pizlo
2016-10-27 13:04 PDT, Build Bot
2016-10-30 11:03 PDT, Filip Pizlo
2016-10-30 12:37 PDT, Build Bot
2016-10-30 17:46 PDT, Build Bot
2016-11-01 21:18 PDT, Filip Pizlo
2016-11-01 21:55 PDT, Filip Pizlo
2016-11-02 07:44 PDT, Filip Pizlo
2016-11-02 14:56 PDT, Filip Pizlo