Changeset 245071 in webkit

Timestamp:

May 8, 2019, 3:19:26 PM (7 years ago)

Author:

ysuzuki@apple.com

Message:

Invalid DFG JIT genereation in high CPU usage state
​https://bugs.webkit.org/show_bug.cgi?id=197453

Reviewed by Saam Barati.

JSTests:

• stress/string-ident-use-clears-abstract-value-if-rope-string-constant-is-held.js: Added.

(trigger):
(main):

Source/JavaScriptCore:

We have a DFG graph like this.

a: JSConstant(rope JSString)
b: CheckStringIdent(Check:StringUse:@a)
... AI think this is unreachable ...

When executing StringUse edge filter onto @a, AbstractValue::filterValueByType clears AbstractValue and makes it None.
This is because @a constant produces SpecString (SpecStringVar | SpecStringIdent) while StringUse edge filter requires
SpecStringIdent. AbstractValue::filterValueByType has an assumption that the JS constant always produces the same
SpeculatedType. So it clears AbstractValue completely.
But this assumption is wrong. JSString can produce SpecStringIdent later if the string is resolved to AtomicStringImpl.
AI think that we always fail. But once the string is resolved to AtomicStringImpl, we pass this check. So we execute
the breakpoint emitted by DFG since DFG think this is unreachable.

In this patch, we just clear the m_value if AbstractValue type filter fails with the held constant, since the constant
may produce a narrower type which can meet the type filter later.

• dfg/DFGAbstractValue.cpp:

(JSC::DFG::AbstractValue::filterValueByType):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/string-ident-use-clears-abstract-value-if-rope-string-constant-is-held.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGAbstractValue.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-05-08  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        Invalid DFG JIT genereation in high CPU usage state
+        https://bugs.webkit.org/show_bug.cgi?id=197453
+
+        Reviewed by Saam Barati.
+
+        * stress/string-ident-use-clears-abstract-value-if-rope-string-constant-is-held.js: Added.
+        (trigger):
+        (main):
+
 2019-05-08  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-05-08  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        Invalid DFG JIT genereation in high CPU usage state
+        https://bugs.webkit.org/show_bug.cgi?id=197453
+
+        Reviewed by Saam Barati.
+
+        We have a DFG graph like this.
+
+            a: JSConstant(rope JSString)
+            b: CheckStringIdent(Check:StringUse:@a)
+            ... AI think this is unreachable ...
+
+        When executing StringUse edge filter onto @a, AbstractValue::filterValueByType clears AbstractValue and makes it None.
+        This is because @a constant produces SpecString (SpecStringVar | SpecStringIdent) while StringUse edge filter requires
+        SpecStringIdent. AbstractValue::filterValueByType has an assumption that the JS constant always produces the same
+        SpeculatedType. So it clears AbstractValue completely.
+        But this assumption is wrong. JSString can produce SpecStringIdent later if the string is resolved to AtomicStringImpl.
+        AI think that we always fail. But once the string is resolved to AtomicStringImpl, we pass this check. So we execute
+        the breakpoint emitted by DFG since DFG think this is unreachable.
+
+        In this patch, we just clear the `m_value` if AbstractValue type filter fails with the held constant, since the constant
+        may produce a narrower type which can meet the type filter later.
+
+        * dfg/DFGAbstractValue.cpp:
+        (JSC::DFG::AbstractValue::filterValueByType):
+
 2019-05-08  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp

@@ -372 +372 @@
     // in any realistic scenario, so we don't do it. Simpler is better.
 
-    if (!!m_type) {
-        // The type is still non-empty. It may be that the new type renders
-        // the value empty because it contravenes the constant value we had.
-        if (m_value && !validateTypeAcceptingBoxedInt52(m_value))
-            clear();
+    if (!m_value)
         return;
-    }
-    
-    // The type has been rendered empty. That means that the value must now be invalid,
-    // as well.
-    ASSERT(!m_value || !validateTypeAcceptingBoxedInt52(m_value));
+
+    if (validateTypeAcceptingBoxedInt52(m_value))
+        return;
+
+    // We assume that the constant value can produce a narrower type at
+    // some point. For example, rope JSString produces SpecString, but
+    // it produces SpecStringIdent once it is resolved to AtomicStringImpl.
+    // We do not make this AbstractValue cleared, but clear the constant
+    // value if validation fails currently.
     m_value = JSValue();
 }