From 640d48255b9980bbab2970bbd563b6f40c4da8c0 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 13 Jul 2023 10:13:30 +0200 Subject: [PATCH] LibJS: Remove unhelpful environment lookup optimization for globals This optimization was no longer helpful after the bug fix for missing invalidation on global delete was introduced in 331f6a9e6, since we now have to check bindings for presence in the global environment every time anyway. Since the bytecode VM now has fast GetGlobal in most cases, let's not even worry about this and just remove the unhelpful "optimization". In fact, removing this is actually an *optimization*, since we avoid a redundant has_binding() check on every global variable access. :^) --- Userland/Libraries/LibJS/AST.cpp | 19 ++++++------------- Userland/Libraries/LibJS/Bytecode/Op.cpp | 19 ++++++------------- .../LibJS/Runtime/EnvironmentCoordinate.h | 1 - .../LibJS/Runtime/GlobalEnvironment.cpp | 5 +---- .../Libraries/LibJS/Runtime/Reference.cpp | 4 ++-- 5 files changed, 15 insertions(+), 33 deletions(-) diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index b6355c4385..f1c3504429 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/Userland/Libraries/LibJS/AST.cpp @@ -1496,19 +1496,12 @@ ThrowCompletionOr Expression::to_reference(Interpreter&) const ThrowCompletionOr Identifier::to_reference(Interpreter& interpreter) const { if (m_cached_environment_coordinate.is_valid()) { - Environment* environment = nullptr; - bool coordinate_screwed_by_delete_in_global_environment = false; - if (m_cached_environment_coordinate.index == EnvironmentCoordinate::global_marker) { - environment = &interpreter.vm().current_realm()->global_environment(); - coordinate_screwed_by_delete_in_global_environment = !TRY(environment->has_binding(string())); - } else { - 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 (!coordinate_screwed_by_delete_in_global_environment && !environment->is_permanently_screwed_by_eval()) { + 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 = {}; diff --git a/Userland/Libraries/LibJS/Bytecode/Op.cpp b/Userland/Libraries/LibJS/Bytecode/Op.cpp index f6866bc061..69c1a10e18 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Op.cpp @@ -389,19 +389,12 @@ ThrowCompletionOr GetVariable::execute_impl(Bytecode::Interpreter& interpr auto get_reference = [&]() -> ThrowCompletionOr { auto const& string = interpreter.current_executable().get_identifier(m_identifier); if (m_cached_environment_coordinate.has_value()) { - Environment* environment = nullptr; - bool coordinate_screwed_by_delete_in_global_environment = false; - if (m_cached_environment_coordinate->index == EnvironmentCoordinate::global_marker) { - environment = &interpreter.vm().current_realm()->global_environment(); - coordinate_screwed_by_delete_in_global_environment = !TRY(environment->has_binding(string)); - } else { - environment = 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 (!coordinate_screwed_by_delete_in_global_environment && !environment->is_permanently_screwed_by_eval()) { + auto environment = 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, vm.in_strict_mode(), m_cached_environment_coordinate }; } m_cached_environment_coordinate = {}; diff --git a/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h b/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h index 679652ec46..7f2cc9c354 100644 --- a/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h +++ b/Userland/Libraries/LibJS/Runtime/EnvironmentCoordinate.h @@ -17,7 +17,6 @@ struct EnvironmentCoordinate { bool is_valid() const { return hops != invalid_marker && index != invalid_marker; } - static constexpr u32 global_marker = 0xffffffff; static constexpr u32 invalid_marker = 0xfffffffe; }; diff --git a/Userland/Libraries/LibJS/Runtime/GlobalEnvironment.cpp b/Userland/Libraries/LibJS/Runtime/GlobalEnvironment.cpp index bb56efa745..08e6a2edcf 100644 --- a/Userland/Libraries/LibJS/Runtime/GlobalEnvironment.cpp +++ b/Userland/Libraries/LibJS/Runtime/GlobalEnvironment.cpp @@ -39,11 +39,8 @@ ThrowCompletionOr GlobalEnvironment::get_this_binding(VM&) const } // 9.1.1.4.1 HasBinding ( N ), https://tc39.es/ecma262/#sec-global-environment-records-hasbinding-n -ThrowCompletionOr GlobalEnvironment::has_binding(DeprecatedFlyString const& name, Optional* out_index) const +ThrowCompletionOr GlobalEnvironment::has_binding(DeprecatedFlyString const& name, Optional*) const { - if (out_index) - *out_index = EnvironmentCoordinate::global_marker; - // 1. Let DclRec be envRec.[[DeclarativeRecord]]. // 2. If ! DclRec.HasBinding(N) is true, return true. if (MUST(m_declarative_record->has_binding(name))) diff --git a/Userland/Libraries/LibJS/Runtime/Reference.cpp b/Userland/Libraries/LibJS/Runtime/Reference.cpp index 4625ad520b..bf7114e7b4 100644 --- a/Userland/Libraries/LibJS/Runtime/Reference.cpp +++ b/Userland/Libraries/LibJS/Runtime/Reference.cpp @@ -67,7 +67,7 @@ ThrowCompletionOr Reference::put_value(VM& vm, Value value) VERIFY(m_base_environment); // c. Return ? base.SetMutableBinding(V.[[ReferencedName]], W, V.[[Strict]]) (see 9.1). - if (m_environment_coordinate.has_value() && m_environment_coordinate->index != EnvironmentCoordinate::global_marker) + if (m_environment_coordinate.has_value()) return static_cast(m_base_environment)->set_mutable_binding_direct(vm, m_environment_coordinate->index, value, m_strict); else return m_base_environment->set_mutable_binding(vm, m_name.as_string(), value, m_strict); @@ -137,7 +137,7 @@ ThrowCompletionOr Reference::get_value(VM& vm) const VERIFY(m_base_environment); // c. Return ? base.GetBindingValue(V.[[ReferencedName]], V.[[Strict]]) (see 9.1). - if (m_environment_coordinate.has_value() && m_environment_coordinate->index != EnvironmentCoordinate::global_marker) + if (m_environment_coordinate.has_value()) return static_cast(m_base_environment)->get_binding_value_direct(vm, m_environment_coordinate->index, m_strict); return m_base_environment->get_binding_value(vm, m_name.as_string(), m_strict); }