Bug 38644 - Optimize access to the global object from a function that uses eval
Summary: Optimize access to the global object from a function that uses eval
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-06 07:10 PDT by Kent Hansen
Modified: 2010-05-10 07:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (38.12 KB, patch)
2010-05-07 16:46 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Hansen 2010-05-06 07:10:32 PDT
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
Comment 1 Oliver Hunt 2010-05-07 16:46:59 PDT
Created attachment 55432 [details]
Patch
Comment 2 WebKit Review Bot 2010-05-07 16:49:21 PDT
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.
Comment 3 Geoffrey Garen 2010-05-07 16:58:52 PDT
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
Comment 4 Oliver Hunt 2010-05-07 17:05:15 PDT
Committed r58986: <http://trac.webkit.org/changeset/58986>
Comment 5 WebKit Review Bot 2010-05-07 18:02:11 PDT
http://trac.webkit.org/changeset/58986 might have broken Qt Linux Release
Comment 6 Eric Seidel (no email) 2010-05-07 19:07:27 PDT
The tree is still on fire from this change:
http://build.webkit.org/console
Comment 7 Eric Seidel (no email) 2010-05-07 19:08:17 PDT
Oliver confirmed over IRC that he's looking into the test failures now.
Comment 8 Kent Hansen 2010-05-10 07:03:03 PDT
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;
     }