Description
Filip Pizlo
2016-10-17 13:31:00 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. 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.
Created attachment 292516 [details]
maybe this could work
Created attachment 292534 [details]
more
Created attachment 292537 [details]
it compiles!
Created attachment 292540 [details]
getting further
It still can't GC without crashing.
Created attachment 292549 [details]
for some reason it actually runs splay
Holy cow this is working.
(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. Created attachment 292550 [details]
shutting down a VM doesn't crash anymore
Comment on attachment 292550 [details]
shutting down a VM doesn't crash anymore
I think I also fixed the memory use 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.
Created attachment 292552 [details]
it works better than I'd expect
It basically passes most tests.
Created attachment 292553 [details]
and now, with a changelog!
Created attachment 292561 [details]
it might work
Fixing more race conditions as I find them.
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 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. 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 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. 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
I think all of those crashes are just because WorkerScriptController needs to grab the JSLock before enabling the watchdog. Created attachment 292571 [details]
another attempt
Created attachment 292572 [details]
fix another crash
Added another JSLockHolder.
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.
Created attachment 292573 [details]
fix another crash
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 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 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 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 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 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. 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.
Created attachment 292631 [details]
fixing more bugs
Fixed a deadlock on VM shutdown.
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.
Created attachment 292634 [details]
more fixes
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.
Created attachment 292639 [details]
fixing more things
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 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. 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
Created attachment 292648 [details]
even more fixes
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 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. 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
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 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
Created attachment 292689 [details]
the most correct patch yet!
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.
Created attachment 292698 [details]
fixed another deadlock
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 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. 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
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 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 Created attachment 292789 [details]
fix more things
I think that worker tests should not timeout anymore.
I fixed another incorrect debug assert.
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. 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. 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 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 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 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 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
Created attachment 292850 [details]
starting to make the runloop hacks work
Created attachment 292875 [details]
HeapTimer hack compiles
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 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 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 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. 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 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 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. Created attachment 292989 [details]
with ThreadSpecific fixes
This fixes one of the indexeddb tests. I think it might fix the other one, too.
Created attachment 293016 [details]
rebased patch
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 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 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 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 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
Created attachment 293027 [details]
try to fix ios build
> 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. 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 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 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
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 (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. 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. Not trying to argue with your approach here -- just summarizing the failures. (In reply to comment #109) > Not trying to argue with your approach here -- just summarizing the failures. Oh, sorry! 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. JetStream looks completely even. I'll check Speedometer and JSBench. (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. ;-) Speedometer is 0.03% faster with 5.8% confidence. (That means neutral.) 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. Created attachment 293357 [details]
rebased
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 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 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 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 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. Created attachment 293636 [details]
the patch
Created attachment 293639 [details]
the patch
Rebased.
I think that this latest patch still has a GC crash in it. This will be fun... 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. Created attachment 293660 [details]
the patch
Updated the changelog.
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 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. (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. Created attachment 293699 [details]
patch for landing
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.
Landed in https://trac.webkit.org/changeset/208306 Seems to have caused crashes when running Speedometer: rdar://problem/29079741 (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 |