Bug 168920 – Make the VM Traps mechanism non-polling for the DFG and FTL.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This is needed so that we can enable the traps checking all the time without any performance penalty.

[Radar WebKit Bug Importer]

<rdar://problem/30738588>

[Mark Lam]

Created attachment 303480 [details]
work in progress.

So far, the patch seems to run well on x86_64 debug builds but is failing watchdog tests on release builds.  The bug is racy.

[Mark Lam]

Created attachment 303728 [details]
proposed patch.

This patch has been tested with release builds on x86_64, x86, armv7, arm64, and a debug build of the Cloop on X86_64.  I still need to do performance testing on it, but I think it's ready for a review.

[Mark Lam]

Comment on attachment 303728 [details]
proposed patch.

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

> Source/JavaScriptCore/API/tests/testapi.c:1138
> +    // mlam TEST
> +    int gazillion = 1;
> +    if (getenv("many"))
> +        gazillion = 1000000;
> +    while (gazillion--) {
> +        failed = testExecutionTimeLimit() || failed;
> +        ASSERT(!failed);
> +        RELEASE_ASSERT(!failed);
> +    }
> +    exit(0);
> +    // mlam TEST */
> +

Eeek.  This is test code that I used to stress test these changes, which I forgot to remove when making this patch.  I will remove this before landing.

[WebKit Commit Bot]

Attachment 303728 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:315:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:365:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 303728 [details]
proposed patch.

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

>> Source/JavaScriptCore/API/tests/testapi.c:1138
>> +
> 
> Eeek.  This is test code that I used to stress test these changes, which I forgot to remove when making this patch.  I will remove this before landing.

Nevermind.  Let me fix this right now so that the EWS bots can do the testing properly.

[Mark Lam]

Created attachment 303729 [details]
proposed patch.

[WebKit Commit Bot]

Attachment 303729 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:315:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:365:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 303732 [details]
proposed patch: fix broken GTK build.

[WebKit Commit Bot]

Attachment 303732 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:315:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:365:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 303739 [details]
proposed patch: remove testapi.c test changes again.

[WebKit Commit Bot]

Attachment 303739 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:315:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:365:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

JSC benchmark runs show that perf is neutral generally except for one regression: LongSpider's crypto-md5 is showing ~29% slow down.  I'll investigate.

[Mark Lam]

(In reply to comment #13)
> JSC benchmark runs show that perf is neutral generally except for one
> regression: LongSpider's crypto-md5 is showing ~29% slow down.  I'll
> investigate.

Interesting.  I tried the following experiments (with the patch already applied):
1. emit nothing for op_check_traps in the baseline JIT: REGRESSED
2. emit no op_check_traps: NO regression.
3. only emit op_check_traps after op_loop_hint: NO regression.
4. emit op_check_traps at code entry and loop hints, but hacked the DFG to only emit the CheckTraps node for traps at loop hints: NO regression.

This means that add the invalidation points at the start of each codeBlock is not free, and is impacting performance.

[Build Bot]

Comment on attachment 303739 [details]
proposed patch: remove testapi.c test changes again.

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

New failing tests:
fast/workers/worker-terminate-forever.html
fast/workers/termination-with-port-messages.html
imported/w3c/web-platform-tests/IndexedDB/idb-binary-key-detached.htm
fetch/fetch-worker-crash.html
fast/workers/wrapper-map-gc.html
media/track/track-in-band-style.html
fast/workers/worker-exception-during-navigation.html
fast/workers/stress-js-execution.html

[Build Bot]

Created attachment 303750 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303739 [details]
proposed patch: remove testapi.c test changes again.

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

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idb-binary-key-roundtrip.htm
fast/workers/termination-early.html
fast/workers/termination-with-port-messages.html
fetch/fetch-worker-crash.html
fast/workers/worker-exception-during-navigation.html
fast/workers/worker-terminate-forever.html
http/tests/websocket/tests/hybi/workers/worker-reload.html

[Build Bot]

Created attachment 303751 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303739 [details]
proposed patch: remove testapi.c test changes again.

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

New failing tests:
fetch/fetch-worker-crash.html
http/tests/websocket/tests/hybi/workers/worker-reload.html

[Build Bot]

Created attachment 303752 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303739 [details]
proposed patch: remove testapi.c test changes again.

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

New failing tests:
fast/workers/worker-terminate-forever.html
fast/workers/termination-with-port-messages.html
imported/w3c/web-platform-tests/IndexedDB/idb-binary-key-detached.htm
fetch/fetch-worker-crash.html
fast/workers/wrapper-map-gc.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
fast/workers/empty-worker-nocrash.html
fast/workers/stress-js-execution.html

[Build Bot]

Created attachment 303753 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Mark Lam]

(In reply to comment #14)
> This means that add the invalidation points at the start of each codeBlock
> is not free, and is impacting performance.

Actually, invalidation points cannot perturb the code gen.  Keith also told me that crypto-md5 is known to be flaky.  Just touching the code can sometimes cause it to show a regression, which then mysteriously disappears later.

Since the regression does not show up on any other benchmarks, I'll plan to land this after the patch gets reviewed.  We can roll out the patch later if the regression proves to be real.

That said, the EWS failures are saying that I have a real bug that I need to fix.  Investigating those now.

[Mark Lam]

Comment on attachment 303739 [details]
proposed patch: remove testapi.c test changes again.

r- while I look into the test failures.

[Mark Lam]

Created attachment 303829 [details]
work in progress.

Fixed 2 missing null checks, and 1 deadlock with WorkerScriptController::scheduleExecutionTermination().  

There is still a race condition window that I need to fix.  Let's try this patch on the EWS while I fix the remaining issue.

[WebKit Commit Bot]

Attachment 303829 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:189:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:214:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:219:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:305:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:313:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:324:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:340:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:346:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:364:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:371:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:382:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:426:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:473:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:501:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 303864 [details]
proposed patch.

I think this patch is good to go, but let's get some EWS testing first.

[WebKit Commit Bot]

Attachment 303864 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:311:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:362:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:491:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 303894 [details]
proposed patch: fixed windows build and added more ChangeLog comments.

[WebKit Commit Bot]

Attachment 303894 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:312:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:363:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:492:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 303894 [details]
proposed patch: fixed windows build and added more ChangeLog comments.

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

This seems like it oughta just work!  You should switch those locker parameters to use const AbstractLocker& since it saves you from doing the reinterpret_cast overload.  Other than that, LGTM.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2993
> +#if ENABLE(SIGNAL_BASED_VM_TRAPS)
> +
> +bool CodeBlock::hasInstalledVMTrapBreakpoints() const
> +{
> +    // This function may be called from a signal handler. We need to be
> +    // careful to not call anything that is not signal handler safe, e.g.
> +    // we should not perturb the refCount of m_jitCode.
> +    if (!JITCode::isOptimizingJIT(jitType()))
> +        return false;
> +    return m_jitCode->dfgCommon()->hasInstalledVMTrapsBreakpoints();
> +}
> +
> +bool CodeBlock::installVMTrapBreakpoints()
> +{
> +    // This function may be called from a signal handler. We need to be
> +    // careful to not call anything that is not signal handler safe, e.g.
> +    // we should not perturb the refCount of m_jitCode.
> +    if (!JITCode::isOptimizingJIT(jitType()))
> +        return false;
> +    m_jitCode->dfgCommon()->installVMTrapBreakpoints();
> +    return true;
> +}
> +
> +#endif // ENABLE(SIGNAL_BASED_VM_TRAPS)
> +

How about when !ENABLE(SIGNAL_BASED_VM_TRAPS), we just make these functions return false.  I think this means less #if's in the header.

(I don't feel strongly about this.)

> Source/JavaScriptCore/bytecode/CodeBlock.h:206
> +#if ENABLE(SIGNAL_BASED_VM_TRAPS)

Then you can get rid of this.

> Source/JavaScriptCore/bytecode/CodeBlock.h:209
> +#endif

And this.

> Source/JavaScriptCore/heap/CodeBlockSet.h:79
>      bool contains(const LockHolder&, void* candidateCodeBlock);
> +    bool contains(const Locker<Lock>& locker, void* candidateCodeBlock)
> +    {
> +        return contains(*reinterpret_cast<const LockHolder*>(&locker), candidateCodeBlock);
> +    }

Just use const AbstractLocker&

> Source/JavaScriptCore/heap/Heap.cpp:2332
> +void Heap::forEachCodeBlockIgnoringJITPlansImpl(Locker<Lock>& locker, const ScopedLambda<bool(CodeBlock*)>& func)

I like to pass const AbstractLocker& in new locking code.  It's usually not interesting to make claims about the type of the locker that was used to grab the lock that this function wants held.  And since we really only have one lock type, it's hardly ever interesting to claim what the type of the lock should be.  const AbstractLocker& helps with that, because it's a base class for all lockers.

> Source/JavaScriptCore/heap/MachineStackMarker.h:139
>      Thread* threadsListHead(const LockHolder&) const { ASSERT(m_registeredThreadsMutex.isLocked()); return m_registeredThreads; }
> +    Thread* threadsListHead(const Locker<Lock>& locker) const { return threadsListHead(*reinterpret_cast<const LockHolder*>(&locker)); }

AbstractLocker

> Source/JavaScriptCore/jit/ExecutableAllocator.h:127
>      bool isValidExecutableMemory(const LockHolder&, void* address);
> +    bool isValidExecutableMemory(const Locker<Lock>& locker, void* address)
> +    {
> +        return isValidExecutableMemory(*reinterpret_cast<const LockHolder*>(&locker), address);
> +    }

AbstractLocker

[Mark Lam]

Comment on attachment 303894 [details]
proposed patch: fixed windows build and added more ChangeLog comments.

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

Thanks for the review.  I've applied the changes.  Will do a test build before committing.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2993
>> +
> 
> How about when !ENABLE(SIGNAL_BASED_VM_TRAPS), we just make these functions return false.  I think this means less #if's in the header.
> 
> (I don't feel strongly about this.)

Done.

>> Source/JavaScriptCore/bytecode/CodeBlock.h:206
>> +#if ENABLE(SIGNAL_BASED_VM_TRAPS)
> 
> Then you can get rid of this.

Done.

>> Source/JavaScriptCore/bytecode/CodeBlock.h:209
>> +#endif
> 
> And this.

Done.

>> Source/JavaScriptCore/heap/CodeBlockSet.h:79
>> +    }
> 
> Just use const AbstractLocker&

Done.

>> Source/JavaScriptCore/heap/Heap.cpp:2332
>> +void Heap::forEachCodeBlockIgnoringJITPlansImpl(Locker<Lock>& locker, const ScopedLambda<bool(CodeBlock*)>& func)
> 
> I like to pass const AbstractLocker& in new locking code.  It's usually not interesting to make claims about the type of the locker that was used to grab the lock that this function wants held.  And since we really only have one lock type, it's hardly ever interesting to claim what the type of the lock should be.  const AbstractLocker& helps with that, because it's a base class for all lockers.

Done.

>> Source/JavaScriptCore/heap/MachineStackMarker.h:139
>> +    Thread* threadsListHead(const Locker<Lock>& locker) const { return threadsListHead(*reinterpret_cast<const LockHolder*>(&locker)); }
> 
> AbstractLocker

Done.

>> Source/JavaScriptCore/jit/ExecutableAllocator.h:127
>> +    }
> 
> AbstractLocker

Done.

[Mark Lam]

Created attachment 303933 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 303933 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:312:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:363:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:492:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Landed in r213652: <http://trac.webkit.org/r213652>.

[Csaba Osztrogonác]

(In reply to comment #35)
> Landed in r213652: <http://trac.webkit.org/r213652>.

Build fixes landed in
https://trac.webkit.org/changeset/213709
https://trac.webkit.org/changeset/213710