Bug 38644 – Optimize access to the global object from a function that uses eval

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kent Hansen]

The insertion of eval in the following script causes a slowdown of ~4x:

a = 123;
(function() {
  var i;
  eval("1");
  for (i = 0; i < 100000; ++i) {
    a; a; a; a; a; a;
  }
})();

Similarly for the following script:

a = 123;
(function() {
  eval("1");
  (function() {
    for (var i = 0; i < 100000; ++i) {
      a; a; a; a; a; a; a; a;
    }
  })();
})();

It would be nice if the eval didn't hurt performance so much.
The optimization should of course not break scripts like the following:

a = 2;
(function(code) {
  eval(code);
  return a + a;
})("var a = 4"); // must return 8, not 4

Or like the following:

a = 2;
print((function() {
  eval("var a = 4");
  return function() {
    return a + a;
  }();
})()); // must return 8, not 4

[Oliver Hunt]

Created attachment 55432 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/interpreter/Interpreter.cpp:2073:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 20 files


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

[Geoffrey Garen]

Comment on attachment 55432 [details]
Patch

I think you can share emit_op_resolve_global_dynamic.

+    requiresDynamicChecks = d()->functionExecutable->usesEval();

Maybe this should be |=, to avoid stomping other functions' results.

"requiresDynamicChecks" sounds a bit too much like "isDynamicScope" to me. How about "mayBecomeDynamicScope"?

r=me

[Oliver Hunt]

Committed r58986: <http://trac.webkit.org/changeset/58986>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58986 might have broken Qt Linux Release

[Eric Seidel (no email)]

The tree is still on fire from this change:
http://build.webkit.org/console

[Eric Seidel (no email)]

Oliver confirmed over IRC that he's looking into the test failures now.

[Kent Hansen]

Comment on attachment 55432 [details]
Patch

Cool, thanks for implementing this.

> +    while (skip--) {
> +        JSObject* o = *iter;
> +        if (o->hasCustomProperties()) {
> +            Identifier& ident = codeBlock->identifier(property);
> +            do {
> +                PropertySlot slot(o);
> +                if (o->getPropertySlot(callFrame, ident, slot)) {
> +                    JSValue result = slot.getValue(callFrame, ident);
> +                    exceptionValue = callFrame->globalData().exception;
> +                    if (exceptionValue)
> +                        return false;
> +                    callFrame->r(dst) = JSValue(result);
> +                    return true;
> +                }
> +                if (iter == end)
> +                    break;
> +                o = *iter;
> +                ++iter;
> +            } while (true);
> +            exceptionValue = createUndefinedVariableError(callFrame, ident, vPC - codeBlock->instructions().begin(), codeBlock);
> +            return false;
> +        }
> +        ++iter;
> +    }

The property is looked up twice in the first scope object with custom properties now, since o is reassigned before ++iter.

Why is the lookup done dynamically for the whole scope chain in case there is just one object with custom properties? Wouldn't it still be possible to cache the property from the outer-most (global) scope and use that one when the dynamic lookup for the inner-most scope fails? This should enable the optimization to do some good in the following program:

a = 2;
(function(code) {
  eval(code);
  (function() {
    for (var i = 0; i < 100000; ++i) {
      a; a; a; a; a; a; a; a;
    }
  })();
})(String.fromCharCode(0x76, 0x61, 0x72, 0x20, 0x63, 0x3D, 0x34)); // "var c=4"

If I replace "eval(code)" by "eval('123')" the caching kicks in, but that's a less interesting use case.

My naive patch:

diff --git a/JavaScriptCore/interpreter/Interpreter.cpp b/JavaScriptCore/interpreter/Interpreter.cpp
index 921dfdf..f777519 100644
--- a/JavaScriptCore/interpreter/Interpreter.cpp
+++ b/JavaScriptCore/interpreter/Interpreter.cpp
@@ -210,23 +210,15 @@ NEVER_INLINE bool Interpreter::resolveGlobalDynamic(CallFrame* callFrame, Instru
         JSObject* o = *iter;
         if (o->hasCustomProperties()) {
             Identifier& ident = codeBlock->identifier(property);
-            do {
-                PropertySlot slot(o);
-                if (o->getPropertySlot(callFrame, ident, slot)) {
-                    JSValue result = slot.getValue(callFrame, ident);
-                    exceptionValue = callFrame->globalData().exception;
-                    if (exceptionValue)
-                        return false;
-                    callFrame->r(dst) = JSValue(result);
-                    return true;
-                }
-                if (iter == end)
-                    break;
-                o = *iter;
-                ++iter;
-            } while (true);
-            exceptionValue = createUndefinedVariableError(callFrame, ident, vPC - codeBlock->instructions().begin(), codeBlock);
-            return false;
+            PropertySlot slot(o);
+            if (o->getPropertySlot(callFrame, ident, slot)) {
+                JSValue result = slot.getValue(callFrame, ident);
+                exceptionValue = callFrame->globalData().exception;
+                if (exceptionValue)
+                    return false;
+                callFrame->r(dst) = JSValue(result);
+                return true;
+            }
         }
         ++iter;
     }