mirror of
https://github.com/LadybirdBrowser/ladybird.git
synced 2025-07-28 11:49:44 +00:00
LibJS: Make mapped arguments objects way less allocation-happy
Some checks are pending
CI / Lagom (arm64, Sanitizer_CI, false, macos-15, macOS, Clang) (push) Waiting to run
CI / Lagom (x86_64, Fuzzers_CI, false, ubuntu-24.04, Linux, Clang) (push) Waiting to run
CI / Lagom (x86_64, Sanitizer_CI, false, ubuntu-24.04, Linux, GNU) (push) Waiting to run
CI / Lagom (x86_64, Sanitizer_CI, true, ubuntu-24.04, Linux, Clang) (push) Waiting to run
Package the js repl as a binary artifact / build-and-package (arm64, macos-15, macOS, macOS-universal2) (push) Waiting to run
Package the js repl as a binary artifact / build-and-package (x86_64, ubuntu-24.04, Linux, Linux-x86_64) (push) Waiting to run
Run test262 and test-wasm / run_and_update_results (push) Waiting to run
Lint Code / lint (push) Waiting to run
Label PRs with merge conflicts / auto-labeler (push) Waiting to run
Push notes / build (push) Waiting to run
Some checks are pending
CI / Lagom (arm64, Sanitizer_CI, false, macos-15, macOS, Clang) (push) Waiting to run
CI / Lagom (x86_64, Fuzzers_CI, false, ubuntu-24.04, Linux, Clang) (push) Waiting to run
CI / Lagom (x86_64, Sanitizer_CI, false, ubuntu-24.04, Linux, GNU) (push) Waiting to run
CI / Lagom (x86_64, Sanitizer_CI, true, ubuntu-24.04, Linux, Clang) (push) Waiting to run
Package the js repl as a binary artifact / build-and-package (arm64, macos-15, macOS, macOS-universal2) (push) Waiting to run
Package the js repl as a binary artifact / build-and-package (x86_64, ubuntu-24.04, Linux, Linux-x86_64) (push) Waiting to run
Run test262 and test-wasm / run_and_update_results (push) Waiting to run
Lint Code / lint (push) Waiting to run
Label PRs with merge conflicts / auto-labeler (push) Waiting to run
Push notes / build (push) Waiting to run
By following the spec to the letter, our mapped arguments objects ended up with many extra GC allocations: - 1 extra Object for the internal [[ParameterMap]]. - 2 extra NativeFunctions for each mapped parameter accessor. - 1 extra Accessor to hold the aforementioned NativeFunctions. This patch removes all those allocations and lets ArgumentsObject model the desired behavior in custom C++ instead of using script primitives. 1.06x speedup on Speedometer's TodoMVC-jQuery.
This commit is contained in:
parent
a71d909b39
commit
a0864dbb26
Notes:
github-actions[bot]
2025-05-11 12:01:34 +00:00
Author: https://github.com/awesomekling
Commit: a0864dbb26
Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/4691
3 changed files with 63 additions and 44 deletions
|
@ -1132,8 +1132,17 @@ Object* create_mapped_arguments_object(VM& vm, FunctionObject& function, Nonnull
|
|||
// 16. Perform ! DefinePropertyOrThrow(obj, "length", PropertyDescriptor { [[Value]]: 𝔽(len), [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }).
|
||||
object->put_direct(realm.intrinsics().mapped_arguments_object_length_offset(), Value(length));
|
||||
|
||||
// OPTIMIZATION: We take a different route here than what the spec suggests.
|
||||
// The spec would have us allocate a new object for the parameter map,
|
||||
// and then populate it with getters and setters for each mapped parameter.
|
||||
// That would be 1 GC allocation for the parameter map and 2 more for each
|
||||
// parameter's getter/setter pair.
|
||||
// Instead, we allocate the ArgumentsObject and let it implement the parameter map
|
||||
// and getter/setter behavior itself without extra GC allocations.
|
||||
|
||||
// 17. Let mappedNames be a new empty List.
|
||||
HashTable<FlyString> mapped_names;
|
||||
HashTable<FlyString> seen_names;
|
||||
Vector<FlyString> mapped_names;
|
||||
|
||||
// 18. Set index to numberOfParameters - 1.
|
||||
// 19. Repeat, while index ≥ 0,
|
||||
|
@ -1143,30 +1152,26 @@ Object* create_mapped_arguments_object(VM& vm, FunctionObject& function, Nonnull
|
|||
auto const& name = formals->parameters()[index].binding.get<NonnullRefPtr<Identifier const>>()->string();
|
||||
|
||||
// b. If name is not an element of mappedNames, then
|
||||
if (mapped_names.contains(name))
|
||||
if (seen_names.contains(name))
|
||||
continue;
|
||||
|
||||
// i. Add name as an element of the list mappedNames.
|
||||
mapped_names.set(name);
|
||||
seen_names.set(name);
|
||||
|
||||
// ii. If index < len, then
|
||||
if (index < length) {
|
||||
// 1. Let g be MakeArgGetter(name, env).
|
||||
// 2. Let p be MakeArgSetter(name, env).
|
||||
// 3. Perform ! map.[[DefineOwnProperty]](! ToString(𝔽(index)), PropertyDescriptor { [[Set]]: p, [[Get]]: g, [[Enumerable]]: false, [[Configurable]]: true }).
|
||||
object->parameter_map().indexed_properties().put(
|
||||
index,
|
||||
Accessor::create(vm,
|
||||
NativeFunction::create(realm, FlyString {}, [&environment, name](VM& vm) -> ThrowCompletionOr<Value> {
|
||||
return MUST(environment.get_binding_value(vm, name, false));
|
||||
}),
|
||||
NativeFunction::create(realm, FlyString {}, [&environment, name](VM& vm) {
|
||||
MUST(environment.set_mutable_binding(vm, name, vm.argument(0), false));
|
||||
return js_undefined();
|
||||
})));
|
||||
if (index >= static_cast<i32>(mapped_names.size()))
|
||||
mapped_names.resize(index + 1);
|
||||
|
||||
mapped_names[index] = name;
|
||||
}
|
||||
}
|
||||
|
||||
object->set_mapped_names(move(mapped_names));
|
||||
|
||||
// 20. Perform ! DefinePropertyOrThrow(obj, @@iterator, PropertyDescriptor { [[Value]]: %Array.prototype.values%, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }).
|
||||
auto array_prototype_values = realm.intrinsics().array_prototype_values_function();
|
||||
object->put_direct(realm.intrinsics().mapped_arguments_object_well_known_symbol_iterator_offset(), array_prototype_values);
|
||||
|
|
|
@ -1,5 +1,5 @@
|
|||
/*
|
||||
* Copyright (c) 2021, Andreas Kling <andreas@ladybird.org>
|
||||
* Copyright (c) 2021-2025, Andreas Kling <andreas@ladybird.org>
|
||||
*
|
||||
* SPDX-License-Identifier: BSD-2-Clause
|
||||
*/
|
||||
|
@ -22,24 +22,27 @@ void ArgumentsObject::initialize(Realm& realm)
|
|||
{
|
||||
Base::initialize(realm);
|
||||
set_has_parameter_map();
|
||||
m_parameter_map = Object::create(realm, nullptr);
|
||||
}
|
||||
|
||||
void ArgumentsObject::visit_edges(Cell::Visitor& visitor)
|
||||
{
|
||||
Base::visit_edges(visitor);
|
||||
visitor.visit(m_environment);
|
||||
visitor.visit(m_parameter_map);
|
||||
}
|
||||
|
||||
bool ArgumentsObject::parameter_map_has(PropertyKey const& key) const
|
||||
{
|
||||
return key.is_number()
|
||||
&& key.as_number() < m_mapped_names.size()
|
||||
&& !m_mapped_names[key.as_number()].is_empty();
|
||||
}
|
||||
|
||||
// 10.4.4.3 [[Get]] ( P, Receiver ), https://tc39.es/ecma262/#sec-arguments-exotic-objects-get-p-receiver
|
||||
ThrowCompletionOr<Value> ArgumentsObject::internal_get(PropertyKey const& property_key, Value receiver, CacheablePropertyMetadata* cacheable_metadata, PropertyLookupPhase phase) const
|
||||
{
|
||||
// 1. Let map be args.[[ParameterMap]].
|
||||
auto& map = *m_parameter_map;
|
||||
|
||||
// 2. Let isMapped be ! HasOwnProperty(map, P).
|
||||
bool is_mapped = MUST(m_parameter_map->has_own_property(property_key));
|
||||
bool is_mapped = parameter_map_has(property_key);
|
||||
|
||||
// 3. If isMapped is false, then
|
||||
if (!is_mapped) {
|
||||
|
@ -47,10 +50,9 @@ ThrowCompletionOr<Value> ArgumentsObject::internal_get(PropertyKey const& proper
|
|||
return Object::internal_get(property_key, receiver, cacheable_metadata, phase);
|
||||
}
|
||||
|
||||
// FIXME: a. Assert: map contains a formal parameter mapping for P.
|
||||
|
||||
// a. Assert: map contains a formal parameter mapping for P.
|
||||
// b. Return ! Get(map, P).
|
||||
return MUST(map.get(property_key));
|
||||
return get_from_parameter_map(property_key);
|
||||
}
|
||||
|
||||
// 10.4.4.4 [[Set]] ( P, V, Receiver ), https://tc39.es/ecma262/#sec-arguments-exotic-objects-set-p-v-receiver
|
||||
|
@ -65,15 +67,14 @@ ThrowCompletionOr<bool> ArgumentsObject::internal_set(PropertyKey const& propert
|
|||
} else {
|
||||
// a. Let map be args.[[ParameterMap]].
|
||||
// b. Let isMapped be ! HasOwnProperty(map, P).
|
||||
is_mapped = MUST(parameter_map().has_own_property(property_key));
|
||||
is_mapped = parameter_map_has(property_key);
|
||||
}
|
||||
|
||||
// 3. If isMapped is true, then
|
||||
if (is_mapped) {
|
||||
// a. Assert: The following Set will succeed, since formal parameters mapped by arguments objects are always writable.
|
||||
|
||||
// b. Perform ! Set(map, P, V, false).
|
||||
MUST(m_parameter_map->set(property_key, value, Object::ShouldThrowExceptions::No));
|
||||
set_in_parameter_map(property_key, value);
|
||||
}
|
||||
|
||||
// 4. Return ? OrdinarySet(args, P, V, Receiver).
|
||||
|
@ -84,10 +85,8 @@ ThrowCompletionOr<bool> ArgumentsObject::internal_set(PropertyKey const& propert
|
|||
ThrowCompletionOr<bool> ArgumentsObject::internal_delete(PropertyKey const& property_key)
|
||||
{
|
||||
// 1. Let map be args.[[ParameterMap]].
|
||||
auto& map = parameter_map();
|
||||
|
||||
// 2. Let isMapped be ! HasOwnProperty(map, P).
|
||||
bool is_mapped = MUST(map.has_own_property(property_key));
|
||||
bool is_mapped = parameter_map_has(property_key);
|
||||
|
||||
// 3. Let result be ? OrdinaryDelete(args, P).
|
||||
bool result = TRY(Object::internal_delete(property_key));
|
||||
|
@ -95,7 +94,7 @@ ThrowCompletionOr<bool> ArgumentsObject::internal_delete(PropertyKey const& prop
|
|||
// 4. If result is true and isMapped is true, then
|
||||
if (result && is_mapped) {
|
||||
// a. Perform ! map.[[Delete]](P).
|
||||
MUST(map.internal_delete(property_key));
|
||||
delete_from_parameter_map(property_key);
|
||||
}
|
||||
|
||||
// 5. Return result.
|
||||
|
@ -114,12 +113,12 @@ ThrowCompletionOr<Optional<PropertyDescriptor>> ArgumentsObject::internal_get_ow
|
|||
|
||||
// 3. Let map be args.[[ParameterMap]].
|
||||
// 4. Let isMapped be ! HasOwnProperty(map, P).
|
||||
bool is_mapped = MUST(m_parameter_map->has_own_property(property_key));
|
||||
bool is_mapped = parameter_map_has(property_key);
|
||||
|
||||
// 5. If isMapped is true, then
|
||||
if (is_mapped) {
|
||||
// a. Set desc.[[Value]] to ! Get(map, P).
|
||||
desc->value = MUST(m_parameter_map->get(property_key));
|
||||
desc->value = get_from_parameter_map(property_key);
|
||||
}
|
||||
|
||||
// 6. Return desc.
|
||||
|
@ -130,10 +129,8 @@ ThrowCompletionOr<Optional<PropertyDescriptor>> ArgumentsObject::internal_get_ow
|
|||
ThrowCompletionOr<bool> ArgumentsObject::internal_define_own_property(PropertyKey const& property_key, PropertyDescriptor const& descriptor, Optional<PropertyDescriptor>* precomputed_get_own_property)
|
||||
{
|
||||
// 1. Let map be args.[[ParameterMap]].
|
||||
auto& map = parameter_map();
|
||||
|
||||
// 2. Let isMapped be ! HasOwnProperty(map, P).
|
||||
bool is_mapped = MUST(map.has_own_property(property_key));
|
||||
bool is_mapped = parameter_map_has(property_key);
|
||||
|
||||
// 3. Let newArgDesc be Desc.
|
||||
auto new_arg_desc = descriptor;
|
||||
|
@ -145,7 +142,7 @@ ThrowCompletionOr<bool> ArgumentsObject::internal_define_own_property(PropertyKe
|
|||
// i. Set newArgDesc to a copy of Desc.
|
||||
new_arg_desc = descriptor;
|
||||
// ii. Set newArgDesc.[[Value]] to ! Get(map, P).
|
||||
new_arg_desc.value = MUST(map.get(property_key));
|
||||
new_arg_desc.value = get_from_parameter_map(property_key);
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -161,19 +158,19 @@ ThrowCompletionOr<bool> ArgumentsObject::internal_define_own_property(PropertyKe
|
|||
// a. If IsAccessorDescriptor(Desc) is true, then
|
||||
if (descriptor.is_accessor_descriptor()) {
|
||||
// i. Perform ! map.[[Delete]](P).
|
||||
MUST(map.internal_delete(property_key));
|
||||
delete_from_parameter_map(property_key);
|
||||
} else {
|
||||
// i. If Desc has a [[Value]] field, then
|
||||
if (descriptor.value.has_value()) {
|
||||
// 1. Assert: The following Set will succeed, since formal parameters mapped by arguments objects are always writable.
|
||||
|
||||
// 2. Perform ! Set(map, P, Desc.[[Value]], false).
|
||||
MUST(map.set(property_key, descriptor.value.value(), Object::ShouldThrowExceptions::No));
|
||||
set_in_parameter_map(property_key, descriptor.value.value());
|
||||
}
|
||||
// ii. If Desc has a [[Writable]] field and Desc.[[Writable]] is false, then
|
||||
if (descriptor.writable == false) {
|
||||
// 1. Perform ! map.[[Delete]](P).
|
||||
MUST(map.internal_delete(property_key));
|
||||
delete_from_parameter_map(property_key);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -182,4 +179,19 @@ ThrowCompletionOr<bool> ArgumentsObject::internal_define_own_property(PropertyKe
|
|||
return true;
|
||||
}
|
||||
|
||||
void ArgumentsObject::delete_from_parameter_map(PropertyKey const& property_key)
|
||||
{
|
||||
m_mapped_names[property_key.as_number()] = FlyString {};
|
||||
}
|
||||
|
||||
Value ArgumentsObject::get_from_parameter_map(PropertyKey const& property_key) const
|
||||
{
|
||||
return MUST(m_environment->get_binding_value(vm(), m_mapped_names[property_key.as_number()], false));
|
||||
}
|
||||
|
||||
void ArgumentsObject::set_in_parameter_map(PropertyKey const& property_key, Value value)
|
||||
{
|
||||
MUST(m_environment->set_mutable_binding(vm(), m_mapped_names[property_key.as_number()], value, false));
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -1,5 +1,5 @@
|
|||
/*
|
||||
* Copyright (c) 2021, Andreas Kling <andreas@ladybird.org>
|
||||
* Copyright (c) 2021-2025, Andreas Kling <andreas@ladybird.org>
|
||||
*
|
||||
* SPDX-License-Identifier: BSD-2-Clause
|
||||
*/
|
||||
|
@ -20,24 +20,26 @@ public:
|
|||
virtual void initialize(Realm&) override;
|
||||
virtual ~ArgumentsObject() override = default;
|
||||
|
||||
Environment& environment() { return m_environment; }
|
||||
|
||||
virtual ThrowCompletionOr<Optional<PropertyDescriptor>> internal_get_own_property(PropertyKey const&) const override;
|
||||
virtual ThrowCompletionOr<bool> internal_define_own_property(PropertyKey const&, PropertyDescriptor const&, Optional<PropertyDescriptor>* precomputed_get_own_property = nullptr) override;
|
||||
virtual ThrowCompletionOr<Value> internal_get(PropertyKey const&, Value receiver, CacheablePropertyMetadata*, PropertyLookupPhase) const override;
|
||||
virtual ThrowCompletionOr<bool> internal_set(PropertyKey const&, Value value, Value receiver, CacheablePropertyMetadata*, PropertyLookupPhase) override;
|
||||
virtual ThrowCompletionOr<bool> internal_delete(PropertyKey const&) override;
|
||||
|
||||
// [[ParameterMap]]
|
||||
Object& parameter_map() { return *m_parameter_map; }
|
||||
void set_mapped_names(Vector<FlyString> mapped_names) { m_mapped_names = move(mapped_names); }
|
||||
|
||||
private:
|
||||
ArgumentsObject(Realm&, Environment&);
|
||||
|
||||
[[nodiscard]] bool parameter_map_has(PropertyKey const&) const;
|
||||
[[nodiscard]] Value get_from_parameter_map(PropertyKey const&) const;
|
||||
void set_in_parameter_map(PropertyKey const&, Value);
|
||||
void delete_from_parameter_map(PropertyKey const&);
|
||||
|
||||
virtual void visit_edges(Cell::Visitor&) override;
|
||||
|
||||
GC::Ref<Environment> m_environment;
|
||||
GC::Ptr<Object> m_parameter_map;
|
||||
Vector<FlyString> m_mapped_names;
|
||||
};
|
||||
|
||||
}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue