AK: Remove excessive hashing caused by FlyString table

Before this change, the global FlyString table looked like this:

    HashMap<StringView, Detail::StringBase>

After this change, we have:

    HashTable<Detail::StringData const*, FlyStringTableHashTraits>

The custom hash traits are used to extract the stored hash from
StringData which avoids having to rehash the StringView repeatedly like
we did before.

This necessitated a handful of smaller changes to make it work.
This commit is contained in:
Andreas Kling 2024-03-23 14:54:23 +01:00
parent 8bfad24708
commit a88799c032
6 changed files with 28 additions and 28 deletions

View file

@ -8,14 +8,21 @@
#include <AK/FlyString.h> #include <AK/FlyString.h>
#include <AK/HashMap.h> #include <AK/HashMap.h>
#include <AK/Singleton.h> #include <AK/Singleton.h>
#include <AK/String.h>
#include <AK/StringData.h>
#include <AK/StringView.h> #include <AK/StringView.h>
#include <AK/Utf8View.h> #include <AK/Utf8View.h>
namespace AK { namespace AK {
struct FlyStringTableHashTraits : public Traits<Detail::StringData const*> {
static u32 hash(Detail::StringData const* string) { return string->hash(); }
static bool equals(Detail::StringData const* a, Detail::StringData const* b) { return *a == *b; }
};
static auto& all_fly_strings() static auto& all_fly_strings()
{ {
static Singleton<HashMap<StringView, Detail::StringBase>> table; static Singleton<HashTable<Detail::StringData const*, FlyStringTableHashTraits>> table;
return *table; return *table;
} }
@ -31,14 +38,19 @@ FlyString::FlyString(String const& string)
return; return;
} }
auto it = all_fly_strings().find(string.bytes_as_string_view()); if (string.m_data->is_fly_string()) {
m_data = string;
return;
}
auto it = all_fly_strings().find(string.m_data);
if (it == all_fly_strings().end()) { if (it == all_fly_strings().end()) {
m_data = string; m_data = string;
all_fly_strings().set(string.m_data);
all_fly_strings().set(string.bytes_as_string_view(), m_data); string.m_data->set_fly_string(true);
string.did_create_fly_string({});
} else { } else {
m_data = it->value; m_data.m_data = *it;
m_data.m_data->ref();
} }
} }
@ -104,9 +116,9 @@ bool FlyString::operator==(char const* string) const
return bytes_as_string_view() == string; return bytes_as_string_view() == string;
} }
void FlyString::did_destroy_fly_string_data(Badge<Detail::StringData>, StringView string_data) void FlyString::did_destroy_fly_string_data(Badge<Detail::StringData>, Detail::StringData const& string_data)
{ {
all_fly_strings().remove(string_data); all_fly_strings().remove(&string_data);
} }
Detail::StringBase FlyString::data(Badge<String>) const Detail::StringBase FlyString::data(Badge<String>) const

View file

@ -48,7 +48,7 @@ public:
[[nodiscard]] int operator<=>(FlyString const& other) const; [[nodiscard]] int operator<=>(FlyString const& other) const;
static void did_destroy_fly_string_data(Badge<Detail::StringData>, StringView); static void did_destroy_fly_string_data(Badge<Detail::StringData>, Detail::StringData const&);
[[nodiscard]] Detail::StringBase data(Badge<String>) const; [[nodiscard]] Detail::StringBase data(Badge<String>) const;
// This is primarily interesting to unit tests. // This is primarily interesting to unit tests.

View file

@ -186,7 +186,7 @@ public:
static ErrorOr<String> from_byte_string(T&&) = delete; static ErrorOr<String> from_byte_string(T&&) = delete;
private: private:
friend FlyString; friend class ::AK::FlyString;
using ShortString = Detail::ShortString; using ShortString = Detail::ShortString;

View file

@ -90,12 +90,6 @@ bool StringBase::operator==(StringBase const& other) const
return bytes() == other.bytes(); return bytes() == other.bytes();
} }
void StringBase::did_create_fly_string(Badge<FlyString>) const
{
VERIFY(!is_short_string());
m_data->set_fly_string(true);
}
ErrorOr<Bytes> StringBase::replace_with_uninitialized_buffer(size_t byte_count) ErrorOr<Bytes> StringBase::replace_with_uninitialized_buffer(size_t byte_count)
{ {
if (byte_count <= MAX_SHORT_STRING_BYTE_COUNT) if (byte_count <= MAX_SHORT_STRING_BYTE_COUNT)

View file

@ -69,8 +69,6 @@ public:
[[nodiscard]] bool operator==(StringBase const&) const; [[nodiscard]] bool operator==(StringBase const&) const;
void did_create_fly_string(Badge<FlyString>) const;
[[nodiscard]] ALWAYS_INLINE FlatPtr raw(Badge<FlyString>) const { return bit_cast<FlatPtr>(m_data); } [[nodiscard]] ALWAYS_INLINE FlatPtr raw(Badge<FlyString>) const { return bit_cast<FlatPtr>(m_data); }
protected: protected:
@ -96,6 +94,9 @@ protected:
ErrorOr<StringBase> substring_from_byte_offset_with_shared_superstring(size_t start, size_t byte_count) const; ErrorOr<StringBase> substring_from_byte_offset_with_shared_superstring(size_t start, size_t byte_count) const;
private: private:
friend class ::AK::String;
friend class ::AK::FlyString;
// NOTE: If the least significant bit of the pointer is set, this is a short string. // NOTE: If the least significant bit of the pointer is set, this is a short string.
static constexpr uintptr_t SHORT_STRING_FLAG = 1; static constexpr uintptr_t SHORT_STRING_FLAG = 1;

View file

@ -44,22 +44,15 @@ public:
void operator delete(void* ptr) void operator delete(void* ptr)
{ {
free(ptr); kfree_sized(ptr, allocation_size_for_string_data(static_cast<StringData const*>(ptr)->m_byte_count));
}
void unref() const
{
if (m_is_fly_string && m_ref_count == 2) {
m_is_fly_string = false; // Otherwise unref from did_destroy_fly_string_data will cause infinite recursion.
FlyString::did_destroy_fly_string_data({}, *this);
}
RefCounted::unref();
} }
~StringData() ~StringData()
{ {
if (m_substring) if (m_substring)
substring_data().superstring->unref(); substring_data().superstring->unref();
if (m_is_fly_string)
FlyString::did_destroy_fly_string_data({}, *this);
} }
SubstringData const& substring_data() const SubstringData const& substring_data() const