Invalid DFG JIT genereation in high CPU usage state
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 May 2019 22:19:26 +0000 (22:19 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 May 2019 22:19:26 +0000 (22:19 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245071 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/string-ident-use-clears-abstract-value-if-rope-string-constant-is-held.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractValue.cpp

index 37c37fb..0cdf46f 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         All prototypes should call didBecomePrototype()
diff --git a/JSTests/stress/string-ident-use-clears-abstract-value-if-rope-string-constant-is-held.js b/JSTests/stress/string-ident-use-clears-abstract-value-if-rope-string-constant-is-held.js
new file mode 100644 (file)
index 0000000..d758d70
--- /dev/null
@@ -0,0 +1,20 @@
+//@ runDefault("--useConcurrentJIT=0")
+function trigger() {
+    var v21 = "test";
+    var v25 = (10)[11];
+    var v27 = 4294967297 + v21;
+    for (let v31 = 1; v31 < 1000; v31++) {
+        ;
+    }
+    v33 = new Float32Array();
+    v34 = v21[v27];
+    v35 = v33[v27];
+}
+
+function main() {
+    for (let i = 0; i < 10000; i++) {
+        trigger();
+    }
+}
+
+main();
index 2d1f711..0e86c59 100644 (file)
@@ -1,3 +1,30 @@
+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>
 
         All prototypes should call didBecomePrototype()
index f9dc4f3..1ca9b74 100644 (file)
@@ -371,17 +371,17 @@ void AbstractValue::filterValueByType()
     // the value, then we could clear both of those things. But that's unlikely to help
     // 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();
 }