[webkit-reviews] review granted: [Bug 179929] [JSC] Implement optimized WeakMap and WeakSet : [Attachment 327727] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 179929: [JSC] Implement optimized WeakMap and WeakSet
https://bugs.webkit.org/show_bug.cgi?id=179929

Attachment 327727: Patch

https://bugs.webkit.org/attachment.cgi?id=327727&action=review




--- Comment #17 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 327727
  --> https://bugs.webkit.org/attachment.cgi?id=327727
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327727&action=review

r=me. Nice patch!

> Source/JavaScriptCore/ChangeLog:17
> +	   2. WeakMapImpl's buffer is allocated in JSValue Gigacage instead
> +	   of auxiliary buffer. This is because we would like to allocate
buffer
> +	   when finalizing GC. At that time, WeakMapImpl prunes dead entries
and
> +	   shrink it if necessary.

Is this because we can't allocate from the GC Heap during finalization? If so
I'd just write that.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:468
> +	   array->putDirectIndex(exec, index >> 1, entry);

let's do / 2. LLVM/GCC will emit the right code

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:95
> +	   ASSERT(m_keyCount > 0);

I vote for RELEASE_ASSERT

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:131
> +// takeSnapshot must not invoke garbage collection since iterating WeakMap
may be modified.
> +template <>
> +void
WeakMapImpl<WeakMapBucket<WeakMapBucketDataKey>>::takeSnapshot(MarkedArgumentBu
ffer& buffer, unsigned limit)
> +{
> +    DisallowGC disallowGC;
> +    unsigned fetched = 0;
> +    forEach([&] (JSObject* key, JSValue) {
> +	   buffer.append(key);
> +	   ++fetched;
> +	   if (limit && fetched >= limit)
> +	       return IterationState::Stop;
> +	   return IterationState::Continue;
> +    });
> +}
> +
> +template <>
> +void
WeakMapImpl<WeakMapBucket<WeakMapBucketDataKeyValue>>::takeSnapshot(MarkedArgum
entBuffer& buffer, unsigned limit)
> +{
> +    DisallowGC disallowGC;
> +    unsigned fetched = 0;
> +    forEach([&] (JSObject* key, JSValue value) {
> +	   buffer.append(key);
> +	   buffer.append(value);
> +	   ++fetched;
> +	   if (limit && fetched >= limit)
> +	       return IterationState::Stop;
> +	   return IterationState::Continue;
> +    });
> +}

You could combine these methods and just have a helper that either adds both
key and value or just key

> Source/JavaScriptCore/runtime/WeakMapImpl.h:58
> +ALWAYS_INLINE uint32_t nextCapacityAfterRemoveBatching(uint32_t capacity,
uint32_t keyCount)

style nit: I'd name this: "nextCapacityAfterBatchRemoval"

> Source/JavaScriptCore/runtime/WeakMapImpl.h:184
> +	   auto buffer = MallocPtr<WeakMapBuffer,
JSValueMalloc>::malloc(allocationSize);
> +	   RELEASE_ASSERT_WITH_MESSAGE(buffer, "If buffer is failed to allocate
during GC, WeakMap becomes completely broken");

Shouldn't this malloc() call ensure we get a result? e.g, it should crash if it
can't allocate. I'd expect ::malloc to guarantee a result, and something like
tryMalloc to perhaps return null.

> Source/JavaScriptCore/runtime/WeakMapImpl.h:228
> +	   DisallowGC disallowGC;

Why would findBucket() GC? Seems like this isn't needed.

> Source/JavaScriptCore/runtime/WeakMapImpl.h:236
> +	   DisallowGC disallowGC;

ditto

> Source/JavaScriptCore/runtime/WeakMapImpl.h:266
> +	   ASSERT(m_keyCount > 0);

I got for RELEASE_ASSERT

> Source/JavaScriptCore/runtime/WeakMapImpl.h:310
> +    IterationState forEach(Functor functor)

Style nit: Why does this return anything? Seems weird. It makes sense for
functor to return this, but I don't see why we'd return that from forEach.

> Source/JavaScriptCore/runtime/WeakMapImpl.h:387
> +	   MallocPtr<WeakMapBufferType, JSValueMalloc> oldBuffer;
> +	   oldBuffer.swap(m_buffer);

Why not  `oldBuffer = WTFMove(m_buffer)`?

> Source/JavaScriptCore/runtime/WeakMapImpl.h:412
> +	       auto* bucket = buffer + index;
> +	       bucket->copyFrom(*entry);

If you do this in the above you don't need to do the add twice. You can just
use bucket.