LibJS+LibWeb: Replace StringOrSymbol usage with PropertyKey
Some checks are pending
CI / Lagom (x86_64, Sanitizer_CI, false, ubuntu-24.04, Linux, GNU) (push) Waiting to run
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, 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

- Avoids unnecessary conversions between StringOrSymbol and PropertyKey
  on the hot path of property access.
- Simplifies the code by removing StringOrSymbol and using PropertyKey
  directly. There was no reason to have a separate StringOrSymbol type
  representing the same data as PropertyKey, just with the index key
  stored as a string.

PropertyKey has been updated to use a tagged pointer instead of a
Variant, so it still occupies 8 bytes, same as StringOrSymbol.

12% improvement on JetStream/gcc-loops.cpp.js
12% improvement on MicroBench/object-assign.js
7% improvement on MicroBench/object-keys.js
This commit is contained in:
Aliaksandr Kalenik 2025-05-15 17:14:00 +03:00 committed by Alexander Kalenik
commit b559965448
Notes: github-actions[bot] 2025-05-17 14:09:31 +00:00
12 changed files with 195 additions and 275 deletions

View file

@ -1,5 +1,6 @@
/*
* Copyright (c) 2020, Andreas Kling <andreas@ladybird.org>
* Copyright (c) 2025, Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
*
* SPDX-License-Identifier: BSD-2-Clause
*/
@ -7,16 +8,13 @@
#pragma once
#include <AK/FlyString.h>
#include <LibGC/Root.h>
#include <LibJS/Runtime/Completion.h>
#include <LibJS/Runtime/StringOrSymbol.h>
#include <LibJS/Runtime/PrimitiveString.h>
#include <LibJS/Runtime/Symbol.h>
namespace JS {
class PropertyKey {
AK_MAKE_DEFAULT_COPYABLE(PropertyKey);
AK_MAKE_DEFAULT_MOVABLE(PropertyKey);
public:
enum class StringMayBeNumber {
Yes,
@ -33,26 +31,61 @@ public:
return TRY(value.to_string(vm));
}
static constexpr uintptr_t NORMAL_STRING_FLAG = 0;
static constexpr uintptr_t SHORT_STRING_FLAG = 1;
static constexpr uintptr_t SYMBOL_FLAG = 2;
static constexpr uintptr_t NUMBER_FLAG = 3;
bool is_string() const { return (m_bits & 3) == NORMAL_STRING_FLAG || (m_bits & 3) == SHORT_STRING_FLAG; }
bool is_number() const { return (m_bits & 3) == NUMBER_FLAG; }
bool is_symbol() const { return (m_bits & 3) == SYMBOL_FLAG; }
PropertyKey() = delete;
PropertyKey(PropertyKey const& other)
{
if (other.is_string())
new (&m_string) FlyString(other.m_string);
else
m_bits = other.m_bits;
}
PropertyKey(PropertyKey&& other) noexcept
{
if (other.is_string())
new (&m_string) FlyString(move(other.m_string));
else
m_bits = exchange(other.m_bits, 0);
}
template<Integral T>
PropertyKey(T index)
: m_data(index)
{
// FIXME: Replace this with requires(IsUnsigned<T>)?
// Needs changes in various places using `int` (but not actually being in the negative range)
VERIFY(index >= 0);
if constexpr (NumericLimits<T>::max() >= NumericLimits<u32>::max()) {
if (index >= NumericLimits<u32>::max()) {
m_data = FlyString { String::number(index) };
new (&m_string) FlyString { String::number(index) };
return;
}
}
m_number = static_cast<u64>(index) << 2 | NUMBER_FLAG;
}
PropertyKey(FlyString string, StringMayBeNumber string_may_be_number = StringMayBeNumber::Yes)
: m_data { try_coerce_into_number(move(string), string_may_be_number) }
{
if (string_may_be_number == StringMayBeNumber::Yes) {
auto view = string.bytes_as_string_view();
if (!view.is_empty() && !(view[0] == '0' && view.length() > 1)) {
auto property_index = view.to_number<u32>(TrimWhitespace::No);
if (property_index.has_value() && property_index.value() < NumericLimits<u32>::max()) {
m_number = static_cast<u64>(property_index.release_value()) << 2 | NUMBER_FLAG;
return;
}
}
}
new (&m_string) FlyString(move(string));
}
PropertyKey(String const& string)
@ -61,66 +94,102 @@ public:
}
PropertyKey(GC::Ref<Symbol> symbol)
: m_data { symbol }
{
m_bits = reinterpret_cast<uintptr_t>(symbol.ptr()) | SYMBOL_FLAG;
}
PropertyKey(StringOrSymbol const& string_or_symbol)
: m_data {
string_or_symbol.is_string()
? Variant<FlyString, GC::Root<Symbol>, u32> { string_or_symbol.as_string() }
: Variant<FlyString, GC::Root<Symbol>, u32> { const_cast<Symbol*>(string_or_symbol.as_symbol()) }
PropertyKey& operator=(PropertyKey const& other)
{
if (this != &other) {
if (is_string())
m_string.~FlyString();
new (this) PropertyKey(other);
}
{
return *this;
}
bool is_number() const { return m_data.has<u32>(); }
bool is_string() const { return m_data.has<FlyString>(); }
bool is_symbol() const { return m_data.has<GC::Root<Symbol>>(); }
PropertyKey& operator=(PropertyKey&& other) noexcept
{
if (this != &other) {
if (is_string())
m_string.~FlyString();
new (this) PropertyKey(move(other));
}
return *this;
}
u32 as_number() const { return m_data.get<u32>(); }
FlyString const& as_string() const { return m_data.get<FlyString>(); }
Symbol const* as_symbol() const { return m_data.get<GC::Root<Symbol>>(); }
~PropertyKey()
{
if (is_string())
m_string.~FlyString();
}
u32 as_number() const
{
VERIFY(is_number());
return m_number >> 2;
}
FlyString const& as_string() const
{
VERIFY(is_string());
return m_string;
}
Symbol const* as_symbol() const
{
VERIFY(is_symbol());
return reinterpret_cast<Symbol const*>(m_bits & ~3ULL);
}
Value to_value(VM& vm) const
{
if (is_string())
return Value { PrimitiveString::create(vm, as_string()) };
if (is_symbol())
return Value { as_symbol() };
return Value { PrimitiveString::create(vm, String::number(as_number())) };
}
String to_string() const
{
VERIFY(!is_symbol());
if (is_string())
return as_string().to_string();
if (is_symbol())
return MUST(as_symbol()->descriptive_string());
return String::number(as_number());
}
StringOrSymbol to_string_or_symbol() const
void visit_edges(Cell::Visitor& visitor) const
{
VERIFY(!is_number());
if (is_string())
return StringOrSymbol(as_string());
return StringOrSymbol(as_symbol());
if (is_symbol())
visitor.visit(const_cast<Symbol*>(as_symbol()));
}
bool operator==(PropertyKey const&) const = default;
bool operator==(PropertyKey const& other) const
{
if (is_string())
return other.is_string() && m_string == other.m_string;
if (is_symbol())
return other.is_symbol() && as_symbol() == other.as_symbol();
if (other.is_number())
return as_number() == other.as_number();
return false;
}
private:
friend Traits<JS::PropertyKey>;
friend Traits<PropertyKey>;
static Variant<FlyString, u32> try_coerce_into_number(FlyString string, StringMayBeNumber string_may_be_number)
{
if (string_may_be_number != StringMayBeNumber::Yes)
return string;
auto view = string.bytes_as_string_view();
if (view.is_empty())
return string;
if (view[0] == '0' && view.length() > 1)
return string;
auto property_index = view.to_number<u32>(TrimWhitespace::No);
if (!property_index.has_value() || property_index.value() >= NumericLimits<u32>::max())
return string;
return property_index.release_value();
}
Variant<FlyString, u32, GC::Root<Symbol>> m_data;
union {
FlyString m_string;
u64 m_number;
Symbol const* m_symbol;
uintptr_t m_bits;
};
};
static_assert(sizeof(PropertyKey) == sizeof(uintptr_t));
}
namespace AK {
@ -129,15 +198,24 @@ template<>
struct Traits<JS::PropertyKey> : public DefaultTraits<JS::PropertyKey> {
static unsigned hash(JS::PropertyKey const& name)
{
return name.m_data.visit(
[](FlyString const& string) { return string.hash(); },
[](GC::Root<JS::Symbol> const& symbol) { return ptr_hash(symbol.ptr()); },
[](u32 const& number) { return int_hash(number); });
if (name.is_string())
return name.as_string().hash();
if (name.is_symbol())
return ptr_hash(name.as_symbol());
if (name.is_number())
return int_hash(name.as_number());
VERIFY_NOT_REACHED();
}
static bool equals(JS::PropertyKey const& a, JS::PropertyKey const& b)
{
return a.m_data == b.m_data;
if (a.is_string())
return b.is_string() && a.as_string() == b.as_string();
if (a.is_symbol())
return b.is_symbol() && a.as_symbol() == b.as_symbol();
if (a.is_number())
return b.is_number() && a.as_number() == b.as_number();
VERIFY_NOT_REACHED();
}
};
@ -147,7 +225,7 @@ struct Formatter<JS::PropertyKey> : Formatter<StringView> {
{
if (property_key.is_number())
return builder.put_u64(property_key.as_number());
return builder.put_string(property_key.to_string_or_symbol().to_display_string());
return builder.put_string(property_key.to_string());
}
};