LibJS: Fast non-local variable access :^)

This patch introduces the "environment coordinate" concept, which
encodes the distance from a variable access to the binding it ends up
resolving to.

EnvironmentCoordinate has two fields:

    - hops:  The number of hops up the lexical environment chain we have
             to make before getting to the resolved binding.

    - index: The index of the resolved binding within its declarative
             environment record.

Whenever a variable lookup resolves somewhere inside a declarative
environment, we now cache the coordinates and reuse them in subsequent
lookups. This is achieved via a coordinate cache in JS::Identifier.

Note that non-strict direct eval() breaks this optimization and so it
will not be performed if the resolved environment has been permanently
screwed by eval().

This makes variable access *significantly* faster. :^)

Userland/Libraries/LibJS/AST.cpp

@@ -1005,7 +1005,20 @@ Reference Expression::to_reference(Interpreter&, GlobalObject&) const
 
 Reference Identifier::to_reference(Interpreter& interpreter, GlobalObject&) const
 {
-    return interpreter.vm().resolve_binding(string());
+    if (m_cached_environment_coordinate.has_value()) {
+        auto* environment = interpreter.vm().running_execution_context().lexical_environment;
+        for (size_t i = 0; i < m_cached_environment_coordinate->hops; ++i)
+            environment = environment->outer_environment();
+        VERIFY(environment);
+        VERIFY(environment->is_declarative_environment());
+        if (!environment->is_permanently_screwed_by_eval())
+            return Reference { *environment, string(), interpreter.vm().in_strict_mode(), m_cached_environment_coordinate };
+        m_cached_environment_coordinate = {};
+    }
+    auto reference = interpreter.vm().resolve_binding(string());
+    if (reference.environment_coordinate().has_value())
+        const_cast<Identifier&>(*this).m_cached_environment_coordinate = reference.environment_coordinate();
+    return reference;
 }
 
 Reference MemberExpression::to_reference(Interpreter& interpreter, GlobalObject& global_object) const

Userland/Libraries/LibJS/AST.h

@@ -17,6 +17,7 @@
 #include <AK/Variant.h>
 #include <AK/Vector.h>
 #include <LibJS/Forward.h>
+#include <LibJS/Runtime/EnvironmentCoordinate.h>
 #include <LibJS/Runtime/PropertyName.h>
 #include <LibJS/Runtime/Reference.h>
 #include <LibJS/Runtime/Value.h>
@@ -984,6 +985,7 @@ private:
     virtual bool is_identifier() const override { return true; }
 
     FlyString m_string;
+    mutable Optional<EnvironmentCoordinate> m_cached_environment_coordinate;
 };
 
 class ClassMethod final : public ASTNode {

Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h

@@ -0,0 +1,18 @@
+/*
+ * Copyright (c) 2021, Andreas Kling <kling@serenityos.org>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#pragma once
+
+#include <LibJS/Forward.h>
+
+namespace JS {
+
+struct EnvironmentCoordinate {
+    size_t hops { 0 };
+    size_t index { 0 };
+};
+
+}

Userland/Libraries/LibJS/Runtime/Reference.cpp

@@ -49,8 +49,8 @@ void Reference::put_value(GlobalObject& global_object, Value value)
 
     VERIFY(m_base_type == BaseType::Environment);
     VERIFY(m_base_environment);
-    if (m_index_in_declarative_environment.has_value())
+    if (m_environment_coordinate.has_value())
-        static_cast<DeclarativeEnvironment*>(m_base_environment)->set_mutable_binding_direct(global_object, m_index_in_declarative_environment.value(), value, m_strict);
+        static_cast<DeclarativeEnvironment*>(m_base_environment)->set_mutable_binding_direct(global_object, m_environment_coordinate->index, value, m_strict);
     else
         m_base_environment->set_mutable_binding(global_object, m_name.as_string(), value, m_strict);
 }
@@ -81,8 +81,8 @@ Value Reference::get_value(GlobalObject& global_object) const
 
     VERIFY(m_base_type == BaseType::Environment);
     VERIFY(m_base_environment);
-    if (m_index_in_declarative_environment.has_value())
+    if (m_environment_coordinate.has_value())
-        return static_cast<DeclarativeEnvironment*>(m_base_environment)->get_binding_value_direct(global_object, m_index_in_declarative_environment.value(), m_strict);
+        return static_cast<DeclarativeEnvironment*>(m_base_environment)->get_binding_value_direct(global_object, m_environment_coordinate->index, m_strict);
     return m_base_environment->get_binding_value(global_object, m_name.as_string(), m_strict);
 }
 

Userland/Libraries/LibJS/Runtime/Reference.h

@@ -8,6 +8,7 @@
 
 #include <AK/String.h>
 #include <LibJS/Runtime/Environment.h>
+#include <LibJS/Runtime/EnvironmentCoordinate.h>
 #include <LibJS/Runtime/PropertyName.h>
 #include <LibJS/Runtime/Value.h>
 
@@ -44,12 +45,12 @@ public:
         }
     }
 
-    Reference(Environment& base, FlyString referenced_name, bool strict = false, Optional<size_t> index_in_declarative_environment = {})
+    Reference(Environment& base, FlyString referenced_name, bool strict = false, Optional<EnvironmentCoordinate> environment_coordinate = {})
         : m_base_type(BaseType::Environment)
         , m_base_environment(&base)
         , m_name(move(referenced_name))
         , m_strict(strict)
-        , m_index_in_declarative_environment(move(index_in_declarative_environment))
+        , m_environment_coordinate(move(environment_coordinate))
     {
     }
 
@@ -119,6 +120,8 @@ public:
 
     bool is_valid_reference() const { return m_name.is_valid(); }
 
+    Optional<EnvironmentCoordinate> environment_coordinate() const { return m_environment_coordinate; }
+
 private:
     void throw_reference_error(GlobalObject&) const;
 
@@ -130,7 +133,7 @@ private:
     PropertyName m_name;
     Value m_this_value;
     bool m_strict { false };
-    Optional<size_t> m_index_in_declarative_environment;
+    Optional<EnvironmentCoordinate> m_environment_coordinate;
 };
 
 }

Userland/Libraries/LibJS/Runtime/VM.cpp

@@ -435,7 +435,7 @@ ThrowCompletionOr<void> VM::iterator_binding_initialization(BindingPattern const
 }
 
 // 9.1.2.1 GetIdentifierReference ( env, name, strict ), https://tc39.es/ecma262/#sec-getidentifierreference
-Reference VM::get_identifier_reference(Environment* environment, FlyString name, bool strict)
+Reference VM::get_identifier_reference(Environment* environment, FlyString name, bool strict, size_t hops)
 {
     // 1. If env is the value null, then
     if (!environment) {
@@ -448,10 +448,14 @@ Reference VM::get_identifier_reference(Environment* environment, FlyString name,
     if (exception())
         return {};
 
+    Optional<EnvironmentCoordinate> environment_coordinate;
+    if (index.has_value())
+        environment_coordinate = EnvironmentCoordinate { .hops = hops, .index = index.value() };
+
     if (exists)
-        return Reference { *environment, move(name), strict, index };
+        return Reference { *environment, move(name), strict, environment_coordinate };
     else
-        return get_identifier_reference(environment->outer_environment(), move(name), strict);
+        return get_identifier_reference(environment->outer_environment(), move(name), strict, hops + 1);
 }
 
 // 9.4.2 ResolveBinding ( name [ , env ] ), https://tc39.es/ecma262/#sec-resolvebinding

Userland/Libraries/LibJS/Runtime/VM.h

@@ -202,7 +202,7 @@ public:
     FlyString unwind_until_label() const { return m_unwind_until_label; }
 
     Reference resolve_binding(FlyString const&, Environment* = nullptr);
-    Reference get_identifier_reference(Environment*, FlyString, bool strict);
+    Reference get_identifier_reference(Environment*, FlyString, bool strict, size_t hops = 0);
 
     template<typename T, typename... Args>
     void throw_exception(GlobalObject& global_object, Args&&... args)

Userland/Libraries/LibJS/Tests/permanently-screwed-by-eval.js

@@ -0,0 +1,13 @@
+test("basic that non-strict direct eval() prevents non-local access caching", () => {
+    function foo(do_eval) {
+        var c = 1;
+        function bar(do_eval) {
+            if (do_eval) eval("var c = 2;");
+            return c;
+        }
+        return bar(do_eval);
+    }
+
+    expect(foo(false)).toBe(1);
+    expect(foo(true)).toBe(2);
+});