RegExpPrototype should check for exceptions after calling toString and doing so shoul...

https://bugs.webkit.org/show_bug.cgi?id=154927

Reviewed by Saam Barati.

While working on regexp optimizations, I found that RegExpPrototype calls toString(), an
effectful operation that could do anything, without then checking for hadException().

So I added a call to hadException().

But that regressed Octane/regexp by 5%!  That's a lot!  It turns out that
exec->hadException() is soooper slow. So, I made it cheaper to check for exceptions from
toString(): there is now a variant called toStringFast() that returns null iff it throws an
exception.

This allowed me to add the exception check without regressing perf.

Note that toString() must retain its old behavior of returning an empty string on exception.
There is just too much code that relies on that behavior.

* runtime/JSCJSValue.cpp:
(JSC::JSValue::isValidCallee):
(JSC::JSValue::toStringSlowCase):
(JSC::JSValue::toWTFStringSlowCase):
* runtime/JSCJSValue.h:
(JSC::JSValue::asValue):
* runtime/JSString.h:
(JSC::JSValue::toString):
(JSC::JSValue::toStringFast):
(JSC::JSValue::toWTFString):
* runtime/RegExpPrototype.cpp:
(JSC::regExpProtoFuncTest):
(JSC::regExpProtoFuncExec):
(JSC::regExpProtoFuncCompile):

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

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 2754911..d3e705d 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,40 @@
+2016-03-02  Filip Pizlo  <fpizlo@apple.com>
+
+        RegExpPrototype should check for exceptions after calling toString and doing so should not be expensive
+        https://bugs.webkit.org/show_bug.cgi?id=154927
+
+        Reviewed by Saam Barati.
+
+        While working on regexp optimizations, I found that RegExpPrototype calls toString(), an
+        effectful operation that could do anything, without then checking for hadException().
+
+        So I added a call to hadException().
+
+        But that regressed Octane/regexp by 5%!  That's a lot!  It turns out that
+        exec->hadException() is soooper slow. So, I made it cheaper to check for exceptions from
+        toString(): there is now a variant called toStringFast() that returns null iff it throws an
+        exception.
+
+        This allowed me to add the exception check without regressing perf.
+
+        Note that toString() must retain its old behavior of returning an empty string on exception.
+        There is just too much code that relies on that behavior.
+
+        * runtime/JSCJSValue.cpp:
+        (JSC::JSValue::isValidCallee):
+        (JSC::JSValue::toStringSlowCase):
+        (JSC::JSValue::toWTFStringSlowCase):
+        * runtime/JSCJSValue.h:
+        (JSC::JSValue::asValue):
+        * runtime/JSString.h:
+        (JSC::JSValue::toString):
+        (JSC::JSValue::toStringFast):
+        (JSC::JSValue::toWTFString):
+        * runtime/RegExpPrototype.cpp:
+        (JSC::regExpProtoFuncTest):
+        (JSC::regExpProtoFuncExec):
+        (JSC::regExpProtoFuncCompile):
+
 2016-03-02  Saam barati  <sbarati@apple.com>
 
         clean up JSObject::isExtensibleInline and JSObject::setPrototypeOfInline, and rename setPrototypeOf to setPrototype

diff --git a/Source/JavaScriptCore/runtime/JSCJSValue.cpp b/Source/JavaScriptCore/runtime/JSCJSValue.cpp
index 82daf75..b9dd0a4 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSCJSValue.cpp
+++ b/Source/JavaScriptCore/runtime/JSCJSValue.cpp
@@ -361,8 +361,14 @@ bool JSValue::isValidCallee()
     return asObject(asCell())->globalObject();
 }
 
-JSString* JSValue::toStringSlowCase(ExecState* exec) const
+JSString* JSValue::toStringSlowCase(ExecState* exec, bool returnEmptyStringOnError) const
 {
+    auto errorValue = [&] () -> JSString* {
+        if (returnEmptyStringOnError)
+            return jsEmptyString(exec);
+        return nullptr;
+    };
+    
     VM& vm = exec->vm();
     ASSERT(!isString());
     if (isInt32()) {
@@ -383,15 +389,18 @@ JSString* JSValue::toStringSlowCase(ExecState* exec) const
         return vm.smallStrings.undefinedString();
     if (isSymbol()) {
         throwTypeError(exec);
-        return jsEmptyString(exec);
+        return errorValue();
     }
 
     ASSERT(isCell());
     JSValue value = asCell()->toPrimitive(exec, PreferString);
-    if (exec->hadException())
-        return jsEmptyString(exec);
+    if (vm.exception())
+        return errorValue();
     ASSERT(!value.isObject());
-    return value.toString(exec);
+    JSString* result = value.toString(exec);
+    if (vm.exception())
+        return errorValue();
+    return result;
 }
 
 String JSValue::toWTFStringSlowCase(ExecState* exec) const

diff --git a/Source/JavaScriptCore/runtime/JSCJSValue.h b/Source/JavaScriptCore/runtime/JSCJSValue.h
index 182c8f4..4ca069e 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSCJSValue.h
+++ b/Source/JavaScriptCore/runtime/JSCJSValue.h
@@ -252,7 +252,8 @@ public:
     // toNumber conversion is expected to be side effect free if an exception has
     // been set in the ExecState already.
     double toNumber(ExecState*) const;
-    JSString* toString(ExecState*) const;
+    JSString* toString(ExecState*) const; // On exception, this returns the empty string.
+    JSString* toStringOrNull(ExecState*) const; // On exception, this returns null, to make exception checks faster.
     Identifier toPropertyKey(ExecState*) const;
     WTF::String toWTFString(ExecState*) const;
     JSObject* toObject(ExecState*) const;
@@ -436,7 +437,7 @@ private:
 
     inline const JSValue asValue() const { return *this; }
     JS_EXPORT_PRIVATE double toNumberSlowCase(ExecState*) const;
-    JS_EXPORT_PRIVATE JSString* toStringSlowCase(ExecState*) const;
+    JS_EXPORT_PRIVATE JSString* toStringSlowCase(ExecState*, bool returnEmptyStringOnError) const;
     JS_EXPORT_PRIVATE WTF::String toWTFStringSlowCase(ExecState*) const;
     JS_EXPORT_PRIVATE JSObject* toObjectSlowCase(ExecState*, JSGlobalObject*) const;
     JS_EXPORT_PRIVATE JSValue toThisSlowCase(ExecState*, ECMAMode) const;

diff --git a/Source/JavaScriptCore/runtime/JSString.h b/Source/JavaScriptCore/runtime/JSString.h
index f273a6e..6dba786 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSString.h
+++ b/Source/JavaScriptCore/runtime/JSString.h
@@ -733,7 +733,16 @@ inline JSString* JSValue::toString(ExecState* exec) const
 {
     if (isString())
         return jsCast<JSString*>(asCell());
-    return toStringSlowCase(exec);
+    bool returnEmptyStringOnError = true;
+    return toStringSlowCase(exec, returnEmptyStringOnError);
+}
+
+inline JSString* JSValue::toStringOrNull(ExecState* exec) const
+{
+    if (isString())
+        return jsCast<JSString*>(asCell());
+    bool returnEmptyStringOnError = false;
+    return toStringSlowCase(exec, returnEmptyStringOnError);
 }
 
 inline String JSValue::toWTFString(ExecState* exec) const

diff --git a/Source/JavaScriptCore/runtime/RegExpPrototype.cpp b/Source/JavaScriptCore/runtime/RegExpPrototype.cpp
index 185549d..db3c16f 100644 (file)
--- a/Source/JavaScriptCore/runtime/RegExpPrototype.cpp
+++ b/Source/JavaScriptCore/runtime/RegExpPrototype.cpp
@@ -99,7 +99,10 @@ EncodedJSValue JSC_HOST_CALL regExpProtoFuncTest(ExecState* exec)
     JSValue thisValue = exec->thisValue();
     if (!thisValue.inherits(RegExpObject::info()))
         return throwVMTypeError(exec);
-    return JSValue::encode(jsBoolean(asRegExpObject(thisValue)->test(exec, exec->argument(0).toString(exec))));
+    JSString* string = exec->argument(0).toStringOrNull(exec);
+    if (!string)
+        return JSValue::encode(jsUndefined());
+    return JSValue::encode(jsBoolean(asRegExpObject(thisValue)->test(exec, string)));
 }
 
 EncodedJSValue JSC_HOST_CALL regExpProtoFuncExec(ExecState* exec)
@@ -107,7 +110,10 @@ EncodedJSValue JSC_HOST_CALL regExpProtoFuncExec(ExecState* exec)
     JSValue thisValue = exec->thisValue();
     if (!thisValue.inherits(RegExpObject::info()))
         return throwVMTypeError(exec);
-    return JSValue::encode(asRegExpObject(thisValue)->exec(exec, exec->argument(0).toString(exec)));
+    JSString* string = exec->argument(0).toStringOrNull(exec);
+    if (!string)
+        return JSValue::encode(jsUndefined());
+    return JSValue::encode(asRegExpObject(thisValue)->exec(exec, string));
 }
 
 EncodedJSValue JSC_HOST_CALL regExpProtoFuncCompile(ExecState* exec)