Bug 163562 – The GC should be in a thread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 292516 [details]
maybe this could work

[Filip Pizlo]

Created attachment 292534 [details]
more

[Filip Pizlo]

Created attachment 292537 [details]
it compiles!

[Filip Pizlo]

Created attachment 292540 [details]
getting further

It still can't GC without crashing.

[Filip Pizlo]

Created attachment 292549 [details]
for some reason it actually runs splay

Holy cow this is working.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 292550 [details]
shutting down a VM doesn't crash anymore

[Filip Pizlo]

Comment on attachment 292550 [details]
shutting down a VM doesn't crash anymore

I think I also fixed the memory use problems.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 292552 [details]
it works better than I'd expect

It basically passes most tests.

[Filip Pizlo]

Created attachment 292553 [details]
and now, with a changelog!

[Filip Pizlo]

Created attachment 292561 [details]
it might work

Fixing more race conditions as I find them.

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Filip Pizlo]

I think all of those crashes are just because WorkerScriptController needs to grab the JSLock before enabling the watchdog.

[Filip Pizlo]

Created attachment 292571 [details]
another attempt

[Filip Pizlo]

Created attachment 292572 [details]
fix another crash

Added another JSLockHolder.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 292573 [details]
fix another crash

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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!

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 292631 [details]
fixing more bugs

Fixed a deadlock on VM shutdown.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 292634 [details]
more fixes

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 292639 [details]
fixing more things

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Filip Pizlo]

Created attachment 292648 [details]
even more fixes

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Created attachment 292689 [details]
the most correct patch yet!

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 292698 [details]
fixed another deadlock

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

(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

[Filip Pizlo]

Created attachment 292789 [details]
fix more things

I think that worker tests should not timeout anymore.

I fixed another incorrect debug assert.

[WebKit Commit Bot]

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.

[Filip Pizlo]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Created attachment 292850 [details]
starting to make the runloop hacks work

[Filip Pizlo]

Created attachment 292875 [details]
HeapTimer hack compiles

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 292989 [details]
with ThreadSpecific fixes

This fixes one of the indexeddb tests.  I think it might fix the other one, too.

[Filip Pizlo]

Created attachment 293016 [details]
rebased patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Created attachment 293027 [details]
try to fix ios build

[Geoffrey Garen]

> 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.

[WebKit Commit Bot]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Build Bot]

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

[Build Bot]

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

[Geoffrey Garen]

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

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Geoffrey Garen]

Not trying to argue with your approach here -- just summarizing the failures.

[Filip Pizlo]

(In reply to comment #109)
> Not trying to argue with your approach here -- just summarizing the failures.

Oh, sorry!

[Filip Pizlo]

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.

[Filip Pizlo]

JetStream looks completely even.

I'll check Speedometer and JSBench.

[Filip Pizlo]

(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. ;-)

[Filip Pizlo]

Speedometer is 0.03% faster with 5.8% confidence.  (That means neutral.)

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 293357 [details]
rebased

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 293636 [details]
the patch

[Filip Pizlo]

Created attachment 293639 [details]
the patch

Rebased.

[Filip Pizlo]

I think that this latest patch still has a GC crash in it.  This will be fun...

[WebKit Commit Bot]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 293660 [details]
the patch

Updated the changelog.

[WebKit Commit Bot]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

(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.

[Geoffrey Garen]

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

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 293699 [details]
patch for landing

[WebKit Commit Bot]

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.

[Filip Pizlo]

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

[Chris Dumez]

Seems to have caused crashes when running Speedometer:
rdar://problem/29079741

[Filip Pizlo]

(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