From 41a072bded25b0993f315bc86e11cae8419f3231 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 7 Oct 2021 01:06:21 +0200 Subject: [PATCH] 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 | 15 ++++++++++++++- Userland/Libraries/LibJS/AST.h | 2 ++ .../LibJS/Runtime/EnvironmentCoordinate.h | 18 ++++++++++++++++++ Userland/Libraries/LibJS/Runtime/Reference.cpp | 8 ++++---- Userland/Libraries/LibJS/Runtime/Reference.h | 9 ++++++--- Userland/Libraries/LibJS/Runtime/VM.cpp | 10 +++++++--- Userland/Libraries/LibJS/Runtime/VM.h | 2 +- .../LibJS/Tests/permanently-screwed-by-eval.js | 13 +++++++++++++ 8 files changed, 65 insertions(+), 12 deletions(-) create mode 100644 Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h create mode 100644 Userland/Libraries/LibJS/Tests/permanently-screwed-by-eval.js diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index 8b2bdfe7cb..8eb93d3ded 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/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(*this).m_cached_environment_coordinate = reference.environment_coordinate(); + return reference; } Reference MemberExpression::to_reference(Interpreter& interpreter, GlobalObject& global_object) const diff --git a/Userland/Libraries/LibJS/AST.h b/Userland/Libraries/LibJS/AST.h index 2baab6fda3..acb8851fbe 100644 --- a/Userland/Libraries/LibJS/AST.h +++ b/Userland/Libraries/LibJS/AST.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -984,6 +985,7 @@ private: virtual bool is_identifier() const override { return true; } FlyString m_string; + mutable Optional m_cached_environment_coordinate; }; class ClassMethod final : public ASTNode { diff --git a/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h b/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h new file mode 100644 index 0000000000..7544908ccb --- /dev/null +++ b/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h @@ -0,0 +1,18 @@ +/* + * Copyright (c) 2021, Andreas Kling + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include + +namespace JS { + +struct EnvironmentCoordinate { + size_t hops { 0 }; + size_t index { 0 }; +}; + +} diff --git a/Userland/Libraries/LibJS/Runtime/Reference.cpp b/Userland/Libraries/LibJS/Runtime/Reference.cpp index b9ec35b37b..3af87c9826 100644 --- a/Userland/Libraries/LibJS/Runtime/Reference.cpp +++ b/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()) - static_cast(m_base_environment)->set_mutable_binding_direct(global_object, m_index_in_declarative_environment.value(), value, m_strict); + if (m_environment_coordinate.has_value()) + static_cast(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()) - return static_cast(m_base_environment)->get_binding_value_direct(global_object, m_index_in_declarative_environment.value(), m_strict); + if (m_environment_coordinate.has_value()) + return static_cast(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); } diff --git a/Userland/Libraries/LibJS/Runtime/Reference.h b/Userland/Libraries/LibJS/Runtime/Reference.h index 6527b9a86e..8749fe5dff 100644 --- a/Userland/Libraries/LibJS/Runtime/Reference.h +++ b/Userland/Libraries/LibJS/Runtime/Reference.h @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -44,12 +45,12 @@ public: } } - Reference(Environment& base, FlyString referenced_name, bool strict = false, Optional index_in_declarative_environment = {}) + Reference(Environment& base, FlyString referenced_name, bool strict = false, Optional 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 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 m_index_in_declarative_environment; + Optional m_environment_coordinate; }; } diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp index 0bd7dcf851..249daf9c72 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.cpp +++ b/Userland/Libraries/LibJS/Runtime/VM.cpp @@ -435,7 +435,7 @@ ThrowCompletionOr 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 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 diff --git a/Userland/Libraries/LibJS/Runtime/VM.h b/Userland/Libraries/LibJS/Runtime/VM.h index ccdd9c2b53..077526ad6c 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.h +++ b/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 void throw_exception(GlobalObject& global_object, Args&&... args) diff --git a/Userland/Libraries/LibJS/Tests/permanently-screwed-by-eval.js b/Userland/Libraries/LibJS/Tests/permanently-screwed-by-eval.js new file mode 100644 index 0000000000..c611fe4441 --- /dev/null +++ b/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); +});