LibJS: Stop lazily coercing numeric PropertyKeys

Lazily coercing might have made sense in the past, but since hashing
and comparing requires the `PropertyKey` to be coerced, and since a
`PropertyKey` will be used to index into a hashmap 99% of the time,
which will hash the `PropertyKey` and use it in comparisons, the
extra complexity and branching produced by lazily coercing has
become more trouble than it is worth.

Remove the lazy coercions, which then also neatly allows us to
switch to a `Variant`-based implementation.

Libraries/LibGC/Root.h

@@ -121,6 +121,18 @@ private:
     RefPtr<RootImpl> m_impl;
 };
 
+template<class T>
+inline bool operator==(Root<T> const& lhs, Root<T> const& rhs)
+{
+    return lhs.ptr() == rhs.ptr();
+}
+
+template<class T>
+inline bool operator!=(Root<T> const& lhs, Root<T> const& rhs)
+{
+    return lhs.ptr() != rhs.ptr();
+}
+
 template<class T>
 inline Root<T> make_root(T* cell, SourceLocation location = SourceLocation::current())
 {

Libraries/LibJS/Runtime/PropertyKey.h

@@ -15,14 +15,10 @@
 namespace JS {
 
 class PropertyKey {
-public:
-    enum class Type : u8 {
-        Invalid,
-        Number,
-        String,
-        Symbol,
-    };
+    AK_MAKE_DEFAULT_COPYABLE(PropertyKey);
+    AK_MAKE_DEFAULT_MOVABLE(PropertyKey);
 
+public:
     enum class StringMayBeNumber {
         Yes,
         No,
@@ -42,130 +38,56 @@ public:
 
     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_string = ByteString::number(index);
-                m_type = Type::String;
-                m_string_may_be_number = false;
+                m_data = DeprecatedFlyString { ByteString::number(index) };
                 return;
             }
         }
-
-        m_type = Type::Number;
-        m_number = index;
     }
 
-    PropertyKey(char const* chars)
-        : m_type(Type::String)
-        , m_string(DeprecatedFlyString(chars))
+    PropertyKey(DeprecatedFlyString string, StringMayBeNumber string_may_be_number = StringMayBeNumber::Yes)
+        : m_data { try_coerce_into_number(move(string), string_may_be_number) }
     {
     }
 
     PropertyKey(ByteString const& string)
-        : m_type(Type::String)
-        , m_string(DeprecatedFlyString(string))
+        : PropertyKey(DeprecatedFlyString(string))
     {
     }
 
-    PropertyKey(FlyString const& string)
-        : m_type(Type::String)
-        , m_string(string.to_deprecated_fly_string())
-    {
-    }
-
-    PropertyKey(DeprecatedFlyString string, StringMayBeNumber string_may_be_number = StringMayBeNumber::Yes)
-        : m_string_may_be_number(string_may_be_number == StringMayBeNumber::Yes)
-        , m_type(Type::String)
-        , m_string(move(string))
+    template<size_t N>
+    PropertyKey(char const (&chars)[N])
+        : PropertyKey(DeprecatedFlyString(chars))
     {
     }
 
     PropertyKey(GC::Ref<Symbol> symbol)
-        : m_type(Type::Symbol)
-        , m_symbol(symbol)
+        : m_data { symbol }
     {
     }
 
     PropertyKey(StringOrSymbol const& string_or_symbol)
-    {
-        if (string_or_symbol.is_string()) {
-            m_string = string_or_symbol.as_string();
-            m_type = Type::String;
-        } else if (string_or_symbol.is_symbol()) {
-            m_symbol = const_cast<Symbol*>(string_or_symbol.as_symbol());
-            m_type = Type::Symbol;
+        : m_data {
+            string_or_symbol.is_string()
+                ? Variant<DeprecatedFlyString, GC::Root<Symbol>, u32> { string_or_symbol.as_string() }
+                : Variant<DeprecatedFlyString, GC::Root<Symbol>, u32> { const_cast<Symbol*>(string_or_symbol.as_symbol()) }
         }
-    }
-
-    ALWAYS_INLINE Type type() const { return m_type; }
-
-    bool is_number() const
     {
-        if (m_type == Type::Number)
-            return true;
-        if (m_type != Type::String || !m_string_may_be_number)
-            return false;
-
-        return const_cast<PropertyKey*>(this)->try_coerce_into_number();
-    }
-    bool is_string() const
-    {
-        if (m_type != Type::String)
-            return false;
-        if (!m_string_may_be_number)
-            return true;
-
-        return !const_cast<PropertyKey*>(this)->try_coerce_into_number();
-    }
-    bool is_symbol() const { return m_type == Type::Symbol; }
-
-    bool try_coerce_into_number()
-    {
-        VERIFY(m_string_may_be_number);
-        if (m_string.is_empty()) {
-            m_string_may_be_number = false;
-            return false;
-        }
-
-        if (char first = m_string.characters()[0]; first < '0' || first > '9') {
-            m_string_may_be_number = false;
-            return false;
-        } else if (m_string.length() > 1 && first == '0') {
-            m_string_may_be_number = false;
-            return false;
-        }
-
-        auto property_index = m_string.to_number<unsigned>(TrimWhitespace::No);
-        if (!property_index.has_value() || property_index.value() == NumericLimits<u32>::max()) {
-            m_string_may_be_number = false;
-            return false;
-        }
-        m_type = Type::Number;
-        m_number = *property_index;
-        return true;
     }
 
-    u32 as_number() const
-    {
-        VERIFY(is_number());
-        return m_number;
-    }
+    bool is_number() const { return m_data.has<u32>(); }
+    bool is_string() const { return m_data.has<DeprecatedFlyString>(); }
+    bool is_symbol() const { return m_data.has<GC::Root<Symbol>>(); }
 
-    DeprecatedFlyString const& as_string() const
-    {
-        VERIFY(is_string());
-        return m_string;
-    }
-
-    Symbol const* as_symbol() const
-    {
-        VERIFY(is_symbol());
-        return m_symbol;
-    }
+    u32 as_number() const { return m_data.get<u32>(); }
+    DeprecatedFlyString const& as_string() const { return m_data.get<DeprecatedFlyString>(); }
+    Symbol const* as_symbol() const { return m_data.get<GC::Root<Symbol>>(); }
 
     ByteString to_string() const
     {
@@ -184,11 +106,23 @@ public:
     }
 
 private:
-    bool m_string_may_be_number { true };
-    Type m_type { Type::Invalid };
-    u32 m_number { 0 };
-    DeprecatedFlyString m_string;
-    GC::Root<Symbol> m_symbol;
+    friend Traits<JS::PropertyKey>;
+
+    static Variant<DeprecatedFlyString, u32> try_coerce_into_number(DeprecatedFlyString string, StringMayBeNumber string_may_be_number)
+    {
+        if (string_may_be_number != StringMayBeNumber::Yes)
+            return string;
+        if (string.is_empty())
+            return string;
+        if (string.starts_with("0"sv) && string.length() != 1)
+            return string;
+        auto property_index = string.to_number<u32>(TrimWhitespace::No);
+        if (!property_index.has_value() || property_index.value() >= NumericLimits<u32>::max())
+            return string;
+        return property_index.release_value();
+    }
+
+    Variant<DeprecatedFlyString, u32, GC::Root<Symbol>> m_data;
 };
 
 }
@@ -199,28 +133,15 @@ template<>
 struct Traits<JS::PropertyKey> : public DefaultTraits<JS::PropertyKey> {
     static unsigned hash(JS::PropertyKey const& name)
     {
-        if (name.is_string())
-            return name.as_string().hash();
-        if (name.is_number())
-            return int_hash(name.as_number());
-        return ptr_hash(name.as_symbol());
+        return name.m_data.visit(
+            [](DeprecatedFlyString const& string) { return string.hash(); },
+            [](GC::Root<JS::Symbol> const& symbol) { return ptr_hash(symbol.ptr()); },
+            [](u32 const& number) { return int_hash(number); });
     }
 
     static bool equals(JS::PropertyKey const& a, JS::PropertyKey const& b)
     {
-        if (a.type() != b.type())
-            return false;
-
-        switch (a.type()) {
-        case JS::PropertyKey::Type::Number:
-            return a.as_number() == b.as_number();
-        case JS::PropertyKey::Type::String:
-            return a.as_string() == b.as_string();
-        case JS::PropertyKey::Type::Symbol:
-            return a.as_symbol() == b.as_symbol();
-        default:
-            VERIFY_NOT_REACHED();
-        }
+        return a.m_data == b.m_data;
     }
 };
 

Libraries/LibWeb/Animations/KeyframeEffect.cpp

@@ -156,7 +156,7 @@ static WebIDL::ExceptionOr<KeyframeType<AL>> process_a_keyframe_like_object(JS::
 
         auto name = input_property.as_string().utf8_string();
         if (name == "all"sv) {
-            all_value = TRY(keyframe_object.get(JS::PropertyKey { name }));
+            all_value = TRY(keyframe_object.get(JS::PropertyKey { "all"sv }));
             for (auto i = to_underlying(CSS::first_longhand_property_id); i <= to_underlying(CSS::last_longhand_property_id); ++i) {
                 auto property = static_cast<CSS::PropertyID>(i);
                 if (CSS::is_animatable_property(property))
@@ -183,7 +183,7 @@ static WebIDL::ExceptionOr<KeyframeType<AL>> process_a_keyframe_like_object(JS::
         // 1. Let raw value be the result of calling the [[Get]] internal method on keyframe input, with property name
         //    as the property key and keyframe input as the receiver.
         // 2. Check the completion record of raw value.
-        JS::PropertyKey key { property_name };
+        JS::PropertyKey key { property_name.to_byte_string() };
         auto raw_value = TRY(keyframe_object.has_property(key)) ? TRY(keyframe_object.get(key)) : *all_value;
 
         using PropertyValuesType = Conditional<AL == AllowLists::Yes, Vector<String>, String>;

Libraries/LibWeb/HTML/HTMLAllCollection.cpp

@@ -118,7 +118,7 @@ Variant<GC::Ref<DOM::HTMLCollection>, GC::Ref<DOM::Element>, Empty> HTMLAllColle
         return Empty {};
 
     // 2. Return the result of getting the "all"-indexed or named element(s) from this, given nameOrIndex.
-    return get_the_all_indexed_or_named_elements(name_or_index.value());
+    return get_the_all_indexed_or_named_elements(name_or_index.value().to_deprecated_fly_string());
 }
 
 // https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#dom-htmlallcollection-nameditem