LibJS: Make GetById and GetByValue avoid get_identifier() in common case

We now defer looking up the various identifiers by IdentifierTableIndex
until the last moment. This allows us to avoid the retrieval in common
cases like when a property access is cached.

Knocks a ~12% item off the profile on https://ventrella.com/Clusters/

Userland/Libraries/LibJS/Bytecode/Executable.h

@@ -86,6 +86,8 @@ public:
     Vector<DeprecatedFlyString> local_variable_names;
     size_t local_index_base { 0 };
 
+    Optional<IdentifierTableIndex> length_identifier;
+
     ByteString const& get_string(StringTableIndex index) const { return string_table->get(index); }
     DeprecatedFlyString const& get_identifier(IdentifierTableIndex index) const { return identifier_table->get(index); }
 

Userland/Libraries/LibJS/Bytecode/Generator.cpp

@@ -453,6 +453,7 @@ CodeGenerationErrorOr<NonnullGCPtr<Executable>> Generator::compile(VM& vm, ASTNo
     executable->source_map = move(source_map);
     executable->local_variable_names = move(local_variable_names);
     executable->local_index_base = number_of_registers + number_of_constants;
+    executable->length_identifier = generator.m_length_identifier;
 
     generator.m_finished = true;
 
@@ -1075,6 +1076,7 @@ CodeGenerationErrorOr<Optional<ScopedOperand>> Generator::emit_named_evaluation_
 void Generator::emit_get_by_id(ScopedOperand dst, ScopedOperand base, IdentifierTableIndex property_identifier, Optional<IdentifierTableIndex> base_identifier)
 {
     if (m_identifier_table->get(property_identifier) == "length"sv) {
+        m_length_identifier = property_identifier;
         emit<Op::GetLength>(dst, base, move(base_identifier), m_next_property_lookup_cache++);
         return;
     }

Userland/Libraries/LibJS/Bytecode/Generator.h

@@ -402,6 +402,8 @@ private:
     bool m_must_propagate_completion { true };
 
     GCPtr<ECMAScriptFunctionObject const> m_function;
+
+    Optional<IdentifierTableIndex> m_length_identifier;
 };
 
 }

Userland/Libraries/LibJS/Bytecode/Interpreter.cpp

@@ -876,6 +876,24 @@ inline void fast_typed_array_set_element(TypedArrayBase& typed_array, u32 index,
     *slot = value;
 }
 
+Completion throw_null_or_undefined_property_get(VM& vm, Value base_value, Optional<IdentifierTableIndex> base_identifier, IdentifierTableIndex property_identifier, Executable const& executable)
+{
+    VERIFY(base_value.is_nullish());
+
+    if (base_identifier.has_value())
+        return vm.throw_completion<TypeError>(ErrorType::ToObjectNullOrUndefinedWithPropertyAndName, executable.get_identifier(property_identifier), base_value, executable.get_identifier(base_identifier.value()));
+    return vm.throw_completion<TypeError>(ErrorType::ToObjectNullOrUndefinedWithProperty, executable.get_identifier(property_identifier), base_value);
+}
+
+Completion throw_null_or_undefined_property_get(VM& vm, Value base_value, Optional<IdentifierTableIndex> base_identifier, Value property, Executable const& executable)
+{
+    VERIFY(base_value.is_nullish());
+
+    if (base_identifier.has_value())
+        return vm.throw_completion<TypeError>(ErrorType::ToObjectNullOrUndefinedWithPropertyAndName, property, base_value, executable.get_identifier(base_identifier.value()));
+    return vm.throw_completion<TypeError>(ErrorType::ToObjectNullOrUndefinedWithProperty, property, base_value);
+}
+
 template<typename BaseType, typename PropertyType>
 ALWAYS_INLINE Completion throw_null_or_undefined_property_access(VM& vm, Value base_value, BaseType const& base_identifier, PropertyType const& property_identifier)
 {
@@ -898,8 +916,7 @@ ALWAYS_INLINE Completion throw_null_or_undefined_property_access(VM& vm, Value b
     return vm.throw_completion<TypeError>(ErrorType::ToObjectNullOrUndefined);
 }
 
-template<typename BaseType, typename PropertyType>
+ALWAYS_INLINE GCPtr<Object> base_object_for_get_impl(VM& vm, Value base_value)
-ALWAYS_INLINE ThrowCompletionOr<NonnullGCPtr<Object>> base_object_for_get(VM& vm, Value base_value, BaseType const& base_identifier, PropertyType const& property_identifier)
 {
     if (base_value.is_object()) [[likely]]
         return base_value.as_object();
@@ -917,8 +934,25 @@ ALWAYS_INLINE ThrowCompletionOr<NonnullGCPtr<Object>> base_object_for_get(VM& vm
     if (base_value.is_symbol())
         return realm.intrinsics().symbol_prototype();
 
+    return nullptr;
+}
+
+ALWAYS_INLINE ThrowCompletionOr<NonnullGCPtr<Object>> base_object_for_get(VM& vm, Value base_value, Optional<IdentifierTableIndex> base_identifier, IdentifierTableIndex property_identifier, Executable const& executable)
+{
+    if (auto base_object = base_object_for_get_impl(vm, base_value))
+        return NonnullGCPtr { *base_object };
+
     // NOTE: At this point this is guaranteed to throw (null or undefined).
-    return throw_null_or_undefined_property_access(vm, base_value, base_identifier, property_identifier);
+    return throw_null_or_undefined_property_get(vm, base_value, base_identifier, property_identifier, executable);
+}
+
+ALWAYS_INLINE ThrowCompletionOr<NonnullGCPtr<Object>> base_object_for_get(VM& vm, Value base_value, Optional<IdentifierTableIndex> base_identifier, Value property, Executable const& executable)
+{
+    if (auto base_object = base_object_for_get_impl(vm, base_value))
+        return NonnullGCPtr { *base_object };
+
+    // NOTE: At this point this is guaranteed to throw (null or undefined).
+    return throw_null_or_undefined_property_get(vm, base_value, base_identifier, property, executable);
 }
 
 enum class GetByIdMode {
@@ -927,7 +961,7 @@ enum class GetByIdMode {
 };
 
 template<GetByIdMode mode = GetByIdMode::Normal>
-inline ThrowCompletionOr<Value> get_by_id(VM& vm, Optional<DeprecatedFlyString const&> const& base_identifier, DeprecatedFlyString const& property, Value base_value, Value this_value, PropertyLookupCache& cache)
+inline ThrowCompletionOr<Value> get_by_id(VM& vm, Optional<IdentifierTableIndex> base_identifier, IdentifierTableIndex property, Value base_value, Value this_value, PropertyLookupCache& cache, Executable const& executable)
 {
     if constexpr (mode == GetByIdMode::Length) {
         if (base_value.is_string()) {
@@ -935,7 +969,7 @@ inline ThrowCompletionOr<Value> get_by_id(VM& vm, Optional<DeprecatedFlyString c
         }
     }
 
-    auto base_obj = TRY(base_object_for_get(vm, base_value, base_identifier, property));
+    auto base_obj = TRY(base_object_for_get(vm, base_value, base_identifier, property, executable));
 
     if constexpr (mode == GetByIdMode::Length) {
         // OPTIMIZATION: Fast path for the magical "length" property on Array objects.
@@ -965,7 +999,7 @@ inline ThrowCompletionOr<Value> get_by_id(VM& vm, Optional<DeprecatedFlyString c
     }
 
     CacheablePropertyMetadata cacheable_metadata;
-    auto value = TRY(base_obj->internal_get(property, this_value, &cacheable_metadata));
+    auto value = TRY(base_obj->internal_get(executable.get_identifier(property), this_value, &cacheable_metadata));
 
     if (cacheable_metadata.type == CacheablePropertyMetadata::Type::OwnProperty) {
         cache = {};
@@ -982,7 +1016,7 @@ inline ThrowCompletionOr<Value> get_by_id(VM& vm, Optional<DeprecatedFlyString c
     return value;
 }
 
-inline ThrowCompletionOr<Value> get_by_value(VM& vm, Optional<DeprecatedFlyString const&> const& base_identifier, Value base_value, Value property_key_value)
+inline ThrowCompletionOr<Value> get_by_value(VM& vm, Optional<IdentifierTableIndex> base_identifier, Value base_value, Value property_key_value, Executable const& executable)
 {
     // OPTIMIZATION: Fast path for simple Int32 indexes in array-like objects.
     if (base_value.is_object() && property_key_value.is_int32() && property_key_value.as_i32() >= 0) {
@@ -1044,7 +1078,7 @@ inline ThrowCompletionOr<Value> get_by_value(VM& vm, Optional<DeprecatedFlyStrin
         }
     }
 
-    auto object = TRY(base_object_for_get(vm, base_value, base_identifier, property_key_value));
+    auto object = TRY(base_object_for_get(vm, base_value, base_identifier, property_key_value, executable));
 
     auto property_key = TRY(property_key_value.to_property_key(vm));
 
@@ -2309,13 +2343,10 @@ ThrowCompletionOr<void> SetVariableBinding::execute_impl(Bytecode::Interpreter&
 
 ThrowCompletionOr<void> GetById::execute_impl(Bytecode::Interpreter& interpreter) const
 {
-    auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier);
-    auto const& property_identifier = interpreter.current_executable().get_identifier(m_property);
-
     auto base_value = interpreter.get(base());
     auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index];
 
-    interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), base_identifier, property_identifier, base_value, base_value, cache)));
+    interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), m_base_identifier, m_property, base_value, base_value, cache, interpreter.current_executable())));
     return {};
 }
 
@@ -2324,18 +2355,17 @@ ThrowCompletionOr<void> GetByIdWithThis::execute_impl(Bytecode::Interpreter& int
     auto base_value = interpreter.get(m_base);
     auto this_value = interpreter.get(m_this_value);
     auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index];
-    interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), {}, interpreter.current_executable().get_identifier(m_property), base_value, this_value, cache)));
+    interpreter.set(dst(), TRY(get_by_id(interpreter.vm(), {}, m_property, base_value, this_value, cache, interpreter.current_executable())));
     return {};
 }
 
 ThrowCompletionOr<void> GetLength::execute_impl(Bytecode::Interpreter& interpreter) const
 {
-    auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier);
-
     auto base_value = interpreter.get(base());
-    auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index];
+    auto& executable = interpreter.current_executable();
+    auto& cache = executable.property_lookup_caches[m_cache_index];
 
-    interpreter.set(dst(), TRY(get_by_id<GetByIdMode::Length>(interpreter.vm(), base_identifier, interpreter.vm().names.length.as_string(), base_value, base_value, cache)));
+    interpreter.set(dst(), TRY(get_by_id<GetByIdMode::Length>(interpreter.vm(), m_base_identifier, *executable.length_identifier, base_value, base_value, cache, executable)));
     return {};
 }
 
@@ -2343,8 +2373,9 @@ ThrowCompletionOr<void> GetLengthWithThis::execute_impl(Bytecode::Interpreter& i
 {
     auto base_value = interpreter.get(m_base);
     auto this_value = interpreter.get(m_this_value);
-    auto& cache = interpreter.current_executable().property_lookup_caches[m_cache_index];
+    auto& executable = interpreter.current_executable();
-    interpreter.set(dst(), TRY(get_by_id<GetByIdMode::Length>(interpreter.vm(), {}, interpreter.vm().names.length.as_string(), base_value, this_value, cache)));
+    auto& cache = executable.property_lookup_caches[m_cache_index];
+    interpreter.set(dst(), TRY(get_by_id<GetByIdMode::Length>(interpreter.vm(), {}, *executable.length_identifier, base_value, this_value, cache, executable)));
     return {};
 }
 
@@ -2699,9 +2730,7 @@ void Await::execute_impl(Bytecode::Interpreter& interpreter) const
 
 ThrowCompletionOr<void> GetByValue::execute_impl(Bytecode::Interpreter& interpreter) const
 {
-    auto base_identifier = interpreter.current_executable().get_identifier(m_base_identifier);
+    interpreter.set(dst(), TRY(get_by_value(interpreter.vm(), m_base_identifier, interpreter.get(m_base), interpreter.get(m_property), interpreter.current_executable())));
-
-    interpreter.set(dst(), TRY(get_by_value(interpreter.vm(), base_identifier, interpreter.get(m_base), interpreter.get(m_property))));
     return {};
 }