This is needed so that we can enable the traps checking all the time without any performance penalty.
<rdar://problem/30738588>
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.
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.
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.
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.
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.
Created attachment 303729 [details] proposed patch.
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.
Created attachment 303732 [details] proposed patch: fix broken GTK build.
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.
Created attachment 303739 [details] proposed patch: remove testapi.c test changes again.
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.
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.
(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.
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
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
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
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
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
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
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
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
(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.
Comment on attachment 303739 [details] proposed patch: remove testapi.c test changes again. r- while I look into the test failures.
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.
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.
Created attachment 303864 [details] proposed patch. I think this patch is good to go, but let's get some EWS testing first.
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.
Created attachment 303894 [details] proposed patch: fixed windows build and added more ChangeLog comments.
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.
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
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.
Created attachment 303933 [details] Patch for landing.
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.
Landed in r213652: <http://trac.webkit.org/r213652>.
(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