Skip to content

Commit

Permalink
[JSC] Remove distinct tail bucket in JSMap / JSSet
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=260117
rdar://113791351

Reviewed by Michael Saboff.

JSMap / JSSet's m_tail can just point to the same entry to the head initially.
And we can keep this head's next() as nullptr.
So,

1. Iterating entries is just doing it until we hit nullptr.
2. Adding a entry is chaining a new entry after m_tail and replace m_tail with this new entry.
3. Removing a entry is unchaining from this linked-list. But we must correctly update m_tail when removing a entry which is pointed by m_tail too.

Then, we do not need to have distinct tail entry for JSMap and JSSet.

* Source/JavaScriptCore/runtime/HashMapImplInlines.h:
(JSC::HashMapImpl<HashMapBucketType>::removeNormalized):
(JSC::HashMapImpl<HashMapBucketType>::clear):
(JSC::HashMapImpl<HashMapBucketType>::setUpHeadAndTail):
(JSC::HashMapImpl<HashMapBucketType>::addNormalizedInternal):
(JSC::HashMapImpl<HashMapBucketType>::rehash):
(JSC::HashMapImpl<HashMapBucketType>::checkConsistency const):

Canonical link: https://commits.webkit.org/266869@main
  • Loading branch information
@Constellation
Constellation committed Aug 14, 2023
1 parent 6b4f259 commit 76449f2
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 62 deletions.
53 changes: 27 additions & 26 deletions Source/JavaScriptCore/runtime/HashMapImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,12 @@ template <typename Data>
class HashMapBucket final : public JSCell {
using Base = JSCell;

template <typename T = Data>
static typename std::enable_if<std::is_same<T, HashMapBucketDataKey>::value, Structure*>::type selectStructure(VM& vm)
{
return vm.hashMapBucketSetStructure.get();
}

template <typename T = Data>
static typename std::enable_if<std::is_same<T, HashMapBucketDataKeyValue>::value, Structure*>::type selectStructure(VM& vm)
static Structure* selectStructure(VM& vm)
{
return vm.hashMapBucketMapStructure.get();
if constexpr (std::is_same_v<Data, HashMapBucketDataKeyValue>)
return vm.hashMapBucketMapStructure.get();
else
return vm.hashMapBucketSetStructure.get();
}

public:
Expand Down Expand Up @@ -124,19 +120,25 @@ class HashMapBucket final : public JSCell {
{
m_prev.set(vm, this, bucket);
}
ALWAYS_INLINE void clearNext()
{
m_next.clear();
}

ALWAYS_INLINE void setKey(VM& vm, JSValue key)
{
m_data.key.set(vm, this, key);
}

template <typename T = Data>
ALWAYS_INLINE typename std::enable_if<std::is_same<T, HashMapBucketDataKeyValue>::value>::type setValue(VM& vm, JSValue value)
ALWAYS_INLINE void setValue(VM& vm, JSValue value)
{
m_data.value.set(vm, this, value);
if constexpr (std::is_same_v<Data, HashMapBucketDataKeyValue>)
m_data.value.set(vm, this, value);
else {
UNUSED_PARAM(vm);
UNUSED_PARAM(value);
}
}
template <typename T = Data>
ALWAYS_INLINE typename std::enable_if<std::is_same<T, HashMapBucketDataKey>::value>::type setValue(VM&, JSValue) { }

ALWAYS_INLINE JSValue key() const { return m_data.key.get(); }

Expand All @@ -152,10 +154,11 @@ class HashMapBucket final : public JSCell {
ALWAYS_INLINE HashMapBucket* prev() const { return m_prev.get(); }

ALWAYS_INLINE bool deleted() const { return !key(); }
ALWAYS_INLINE void makeDeleted(VM& vm)
ALWAYS_INLINE void makeDeleted()
{
setKey(vm, JSValue());
setValue(vm, JSValue());
m_data.key.clear();
if constexpr (std::is_same_v<Data, HashMapBucketDataKeyValue>)
m_data.value.clear();
}

static ptrdiff_t offsetOfKey()
Expand All @@ -174,16 +177,14 @@ class HashMapBucket final : public JSCell {
return OBJECT_OFFSETOF(HashMapBucket, m_next);
}

template <typename T = Data>
ALWAYS_INLINE static typename std::enable_if<std::is_same<T, HashMapBucketDataKeyValue>::value, JSValue>::type extractValue(const HashMapBucket& bucket)
{
return bucket.value();
}

template <typename T = Data>
ALWAYS_INLINE static typename std::enable_if<std::is_same<T, HashMapBucketDataKey>::value, JSValue>::type extractValue(const HashMapBucket&)
ALWAYS_INLINE static JSValue extractValue(const HashMapBucket& bucket)
{
return JSValue();
if constexpr (std::is_same_v<Data, HashMapBucketDataKeyValue>)
return bucket.value();
else {
UNUSED_PARAM(bucket);
return JSValue();
}
}

private:
Expand Down
84 changes: 48 additions & 36 deletions Source/JavaScriptCore/runtime/HashMapImplInlines.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,14 @@ ALWAYS_INLINE bool HashMapImpl<HashMapBucketType>::removeNormalized(JSGlobalObje

VM& vm = getVM(globalObject);
HashMapBucketType* impl = *bucket;
impl->next()->setPrev(vm, impl->prev());
impl->prev()->setNext(vm, impl->next());
impl->makeDeleted(vm);
HashMapBucketType* prev = impl->prev();
// If this bucket is the last one in the chain, then we just mark it as deleted.
// It is even possible that this one is in m_tail, but it is OK: we handle it when inserting a new entry.
if (impl->next()) {
impl->next()->setPrev(vm, prev);
prev->setNext(vm, impl->next());
}
impl->makeDeleted();

*bucket = deletedValue();

Expand All @@ -310,16 +315,15 @@ ALWAYS_INLINE void HashMapImpl<HashMapBucketType>::clear(VM& vm)
m_deleteCount = 0;
HashMapBucketType* head = m_head.get();
HashMapBucketType* bucket = m_head->next();
HashMapBucketType* tail = m_tail.get();
while (bucket != tail) {
while (bucket) {
HashMapBucketType* next = bucket->next();
// We restart each iterator by pointing it to the head of the list.
bucket->setNext(vm, head);
bucket->makeDeleted(vm);
bucket->makeDeleted();
bucket = next;
}
m_head->setNext(vm, m_tail.get());
m_tail->setPrev(vm, m_head.get());
head->clearNext();
m_tail.set(vm, this, head);
m_buffer.clear();
m_capacity = 0;
checkConsistency();
Expand All @@ -328,13 +332,12 @@ ALWAYS_INLINE void HashMapImpl<HashMapBucketType>::clear(VM& vm)
template <typename HashMapBucketType>
ALWAYS_INLINE void HashMapImpl<HashMapBucketType>::setUpHeadAndTail(VM& vm)
{
m_head.set(vm, this, HashMapBucketType::create(vm));
m_tail.set(vm, this, HashMapBucketType::create(vm));

m_head->setNext(vm, m_tail.get());
m_tail->setPrev(vm, m_head.get());
auto* head = HashMapBucketType::create(vm);
m_head.set(vm, this, head);
m_tail.set(vm, this, head);
ASSERT(!m_head->prev());
ASSERT(!m_head->next());
ASSERT(m_head->deleted());
ASSERT(m_tail->deleted());
}

template <typename HashMapBucketType>
Expand Down Expand Up @@ -413,19 +416,27 @@ ALWAYS_INLINE HashMapBucketType* HashMapImpl<HashMapBucketType>::addNormalizedIn
index = (index + 1) & mask;
}

HashMapBucketType* newEntry = m_tail.get();
buffer[index] = newEntry;
newEntry->setKey(vm, key);
newEntry->setValue(vm, value);
ASSERT(!newEntry->deleted());
auto* oldTail = m_tail.get();
HashMapBucketType* newTail = HashMapBucketType::create(vm);
newTail->setKey(vm, key);
newTail->setValue(vm, value);
ASSERT(!newTail->deleted());
buffer[index] = newTail;
oldTail->setNext(vm, newTail);

if (!oldTail->deleted() || oldTail == m_head.get())
newTail->setPrev(vm, oldTail);
else {
auto* prev = oldTail->prev();
ASSERT(prev);
newTail->setPrev(vm, prev);
prev->setNext(vm, newTail);
}
m_tail.set(vm, this, newTail);
newTail->setPrev(vm, newEntry);
ASSERT(newTail->deleted());
newEntry->setNext(vm, newTail);
ASSERT(!newTail->next());

++m_keyCount;
return newEntry;
return newTail;
}

template <typename HashMapBucketType>
Expand Down Expand Up @@ -471,21 +482,22 @@ void HashMapImpl<HashMapBucketType>::rehash(JSGlobalObject* globalObject, Rehash
}

HashMapBucketType* iter = m_head->next();
HashMapBucketType* end = m_tail.get();
const uint32_t mask = m_capacity - 1;
RELEASE_ASSERT(!(m_capacity & (m_capacity - 1)));
HashMapBucketType** buffer = this->buffer();
while (iter != end) {
uint32_t index = jsMapHashForAlreadyHashedValue(globalObject, vm, iter->key()) & mask;
EXCEPTION_ASSERT_WITH_MESSAGE(!scope.exception(), "All keys should already be hashed before, so this should not throw because it won't resolve ropes.");
{
HashMapBucketType* bucket = buffer[index];
while (!isEmpty(bucket)) {
index = (index + 1) & mask;
bucket = buffer[index];
while (iter) {
if (!iter->deleted()) {
uint32_t index = jsMapHashForAlreadyHashedValue(globalObject, vm, iter->key()) & mask;
EXCEPTION_ASSERT_WITH_MESSAGE(!scope.exception(), "All keys should already be hashed before, so this should not throw because it won't resolve ropes.");
{
HashMapBucketType* bucket = buffer[index];
while (!isEmpty(bucket)) {
index = (index + 1) & mask;
bucket = buffer[index];
}
}
buffer[index] = iter;
}
buffer[index] = iter;
iter = iter->next();
}

Expand All @@ -499,10 +511,10 @@ ALWAYS_INLINE void HashMapImpl<HashMapBucketType>::checkConsistency() const
{
if (ASSERT_ENABLED) {
HashMapBucketType* iter = m_head->next();
HashMapBucketType* end = m_tail.get();
uint32_t size = 0;
while (iter != end) {
++size;
while (iter) {
if (!iter->deleted())
++size;
iter = iter->next();
}
ASSERT_UNUSED(size, size == m_keyCount);
Expand Down

0 comments on commit 76449f2

Please sign in to comment.