Bug 168842 – Introduce a VM Traps mechanism and refactor Watchdog to use it.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This introduces the ability to fire asynchronous traps on the JS thread.  For now, we'll only be using this for the Watchdog mechanism, and asynchronous termination.  Later on, we can use this to add support for debugger breaks.

[Mark Lam]

Such a trapping mechanism requires polling of some flag from LLInt and baseline JITted code (assuming we don't want to implement something more exotic like an invalidation point for the baseline JIT).  We can implement this polling in 2 ways:

1. Add a new polling check into the bytecode stream.

   This is how the pre-existing Watchdog code work.

2. Overload stack checking (at the start of functions) and the JIT counter check (at loop hints) as a trigger for trap checks.

   This comes at a price of added complexity, but would ensure no loss of performance at the LLInt and baseline JIT tiers.

I did a jsc only benchmark run comparing a vm without op_watchdog vs a vm with op_watchdog emitted minus a hack to make the DFG and FTL layers emit no code for op_watchdog.  The DFG and FTL hack is necessary because our plan is to handle the trap check at the DFG and FTL tiers using invalidation points.  This configuration simulates what we would get in the final implementation of (1) above.

With that, I saw that performance was neutral.  Based on this, I will go with implementation (1) to keep things simple.  We can revisit changing to using (2) later if we end up observing a performance difference due to the extra polling of (1).

[Radar WebKit Bug Importer]

<rdar://problem/30703107>

[Mark Lam]

Created attachment 302859 [details]
proposed patch.

Let's get some EWS testing.

[WebKit Commit Bot]

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


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: 1 in 36 files


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

[Build Bot]

Comment on attachment 302859 [details]
proposed patch.

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

New failing tests:
fast/dom/timer-throttling-hidden-page.html

[Build Bot]

Created attachment 302867 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Mark Lam]

Created attachment 302869 [details]
proposed patch 2 (improved).

[WebKit Commit Bot]

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


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: 1 in 37 files


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

[Filip Pizlo]

Comment on attachment 302869 [details]
proposed patch 2 (improved).

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:5554
> -        ASSERT(m_jit.vm()->watchdog());
> +    case CheckTraps: {
>          GPRTemporary unused(this);
>          GPRReg unusedGPR = unused.gpr();
>          
> -        JITCompiler::Jump timerDidFire = m_jit.branchTest8(JITCompiler::NonZero,
> -            JITCompiler::AbsoluteAddress(m_jit.vm()->watchdog()->timerDidFireAddress()));
> +        JITCompiler::Jump needTrapHandling = m_jit.branchTest8(JITCompiler::NonZero,
> +            JITCompiler::AbsoluteAddress(m_jit.vm()->needTrapHandlingAddress()));
>          
> -        addSlowPathGenerator(slowPathCall(timerDidFire, this, operationHandleWatchdogTimer, unusedGPR));
> +        addSlowPathGenerator(slowPathCall(needTrapHandling, this, operationHandleTraps, unusedGPR));
>          break;

Make common with 64-bit code.

Create a SpeculativeJIT::compileCheckTraps(Node*) and put it in SpeculativeJIT.cpp, and then call it here.

> Source/JavaScriptCore/runtime/VM.h:723
> +    class Traps {
> +    public:
> +        enum EventType {
> +            // Sorted in servicing priority order from highest to lowest.
> +            NeedTermination,
> +            NeedWatchdogCheck,
> +            NumberOfEventTypes, // This entry must be last in this list.
> +            Invalid
> +        };
> +
> +        bool needTrapHandling() { return m_needTrapHandling; }
> +        void* needTrapHandlingAddress() { return &m_needTrapHandling; }
> +
> +        JS_EXPORT_PRIVATE void fireTrap(EventType);
> +
> +        bool takeTrap(EventType);
> +        EventType takeTopPriorityTrap();
> +
> +    private:
> +        VM& vm() const;
> +
> +        bool hasTrapForEvent(Locker<Lock>&, EventType eventType)
> +        {
> +            ASSERT(eventType < NumberOfEventTypes);
> +            return (m_trapsBitField & (1 << eventType));
> +        }
> +        void setTrapForEvent(Locker<Lock>&, EventType eventType)
> +        {
> +            ASSERT(eventType < NumberOfEventTypes);
> +            m_trapsBitField |= (1 << eventType);
> +        }
> +        void clearTrapForEvent(Locker<Lock>&, EventType eventType)
> +        {
> +            ASSERT(eventType < NumberOfEventTypes);
> +            m_trapsBitField &= ~(1 << eventType);
> +        }
> +
> +        Lock m_lock;
> +        union {
> +            uint8_t m_needTrapHandling { false };
> +            uint8_t m_trapsBitField;
> +        };
> +
> +        friend class LLIntOffsetsExtractor;
> +    };

Make this not nested in VM.  Put it in a separate header/cpp file (even though you will include the header in VM.h)

> Source/JavaScriptCore/runtime/Watchdog.cpp:160
> +    // We need to ensure that the Watchdog outlives the timer.
> +    // The timer callback lambda below will deref later to match this ref.
> +    // For the same reason, the timer may also outlive the VM that the Watchdog operates on.
> +    // So, we always need to null check m_vm before using it. The VM will notify the Watchdog
> +    // via willDestroyVM() before it goes away.
> +    this->ref();

Say something like this instead:

RefPtr<Watchdog> protectedThis = this;

> Source/JavaScriptCore/runtime/Watchdog.cpp:163
>      m_timerQueue->dispatchAfter(std::chrono::nanoseconds(timeLimit), [this] {

Change [this] to [this, protectedThis]

Or even better:

[this, RefPtr<WatchDog> protectedThis = this]

> Source/JavaScriptCore/runtime/Watchdog.cpp:171
>          deref();

Don't need this

[Mark Lam]

Created attachment 302889 [details]
patch for landing.

Thanks for the review.  I've applied the requested changes.

[WebKit Commit Bot]

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


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: 1 in 43 files


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

[WebKit Commit Bot]

Comment on attachment 302889 [details]
patch for landing.

Clearing flags on attachment: 302889

Committed r213107: <http://trac.webkit.org/changeset/213107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.