Bug 179929 - [JSC] Implement optimized WeakMap and WeakSet
Summary: [JSC] Implement optimized WeakMap and WeakSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 180015
  Show dependency treegraph
 
Reported: 2017-11-21 14:27 PST by Yusuke Suzuki
Modified: 2017-12-14 03:40 PST (History)
12 users (show)

See Also:


Attachments
Patch (78.77 KB, patch)
2017-11-23 10:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (94.36 KB, patch)
2017-11-24 03:04 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (94.46 KB, patch)
2017-11-24 03:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (94.46 KB, patch)
2017-11-24 03:19 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (3.00 MB, application/zip)
2017-11-24 04:54 PST, EWS Watchlist
no flags Details
Patch (95.97 KB, patch)
2017-11-24 05:28 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (97.14 KB, patch)
2017-11-24 05:47 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (3.03 MB, application/zip)
2017-11-24 07:22 PST, EWS Watchlist
no flags Details
Patch (111.20 KB, patch)
2017-11-24 09:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (111.91 KB, patch)
2017-11-24 09:19 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (111.45 KB, patch)
2017-11-27 22:01 PST, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-11-21 14:27:16 PST
We should do the similar thing to JSMap and JSSet.

Notes.

1. WeakMap does not have iterators. Thus, we do not need to maintain bucket's links.
2. Since we do not have iterators, we do not need to have bucket as cell.
3. Key is only JSObject*.
Comment 1 Yusuke Suzuki 2017-11-23 10:46:11 PST
Created attachment 327510 [details]
Patch
Comment 2 Yusuke Suzuki 2017-11-24 03:04:39 PST
Created attachment 327532 [details]
Patch
Comment 3 Yusuke Suzuki 2017-11-24 03:09:14 PST
Created attachment 327533 [details]
Patch
Comment 4 Yusuke Suzuki 2017-11-24 03:17:07 PST
Comment on attachment 327533 [details]
Patch

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:513
> +    weakSet->forEach([&] (JSObject* key, JSValue) {

Not good. We should not have such interface in WeakMap since functor can cause GC.
I think this is pre-existing bug. Old WeakMap implementaiton is also broken.
If we would like to iterate it, once we should buffer all the entries and we should put it to the result object.
Comment 5 Yusuke Suzuki 2017-11-24 03:19:57 PST
Created attachment 327535 [details]
Patch
Comment 6 EWS Watchlist 2017-11-24 04:54:13 PST
Comment on attachment 327535 [details]
Patch

Attachment 327535 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5348290

New failing tests:
js/dom/basic-weakset.html
js/dom/basic-weakmap.html
Comment 7 EWS Watchlist 2017-11-24 04:54:14 PST
Created attachment 327542 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Yusuke Suzuki 2017-11-24 05:28:28 PST
Created attachment 327545 [details]
Patch
Comment 9 Yusuke Suzuki 2017-11-24 05:47:52 PST
Created attachment 327547 [details]
Patch
Comment 10 EWS Watchlist 2017-11-24 07:22:18 PST
Comment on attachment 327547 [details]
Patch

Attachment 327547 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5349509

New failing tests:
js/dom/basic-weakset.html
js/dom/basic-weakmap.html
Comment 11 EWS Watchlist 2017-11-24 07:22:19 PST
Created attachment 327549 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Yusuke Suzuki 2017-11-24 09:09:55 PST
Created attachment 327551 [details]
Patch
Comment 13 Yusuke Suzuki 2017-11-24 09:19:02 PST
Created attachment 327552 [details]
Patch
Comment 14 Yusuke Suzuki 2017-11-27 22:01:34 PST
Created attachment 327727 [details]
Patch
Comment 15 Yusuke Suzuki 2017-11-30 15:58:58 PST
Ping?
Comment 16 Yusuke Suzuki 2017-12-09 12:01:39 PST
There is ongoing effort removing UnconditionalFinalizer & WeakReferenceHarvester. I think we can apply them to WeakMap/WeakSet.
Comment 17 Saam Barati 2017-12-12 14:15:25 PST
Comment on attachment 327727 [details]
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(MarkedArgumentBuffer& 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(MarkedArgumentBuffer& 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.
Comment 18 Yusuke Suzuki 2017-12-12 18:44:11 PST
Comment on attachment 327727 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/ChangeLog:17
>> +        shrink it if necessary.
> 
> Is this because we can't allocate from the GC Heap during finalization? If so I'd just write that.

Yeah, added this description.

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:468
>> +        array->putDirectIndex(exec, index >> 1, entry);
> 
> let's do / 2. LLVM/GCC will emit the right code

Fixed.

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

Replaced!

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:131
>> +}
> 
> You could combine these methods and just have a helper that either adds both key and value or just key

Yeah, sounds nice to me too. Fixed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:58
>> +ALWAYS_INLINE uint32_t nextCapacityAfterRemoveBatching(uint32_t capacity, uint32_t keyCount)
> 
> style nit: I'd name this: "nextCapacityAfterBatchRemoval"

Fixed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:184
>> +        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.

Yeah, I removed this. This assertion should be done in malloc side.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:228
>> +        DisallowGC disallowGC;
> 
> Why would findBucket() GC? Seems like this isn't needed.

This is different from DisableGC. This DisallowGC ensures that the following operation does not cause GC. If operation causing GC occurs, assertion fires.

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

Ditto.

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

Replaced.

>> 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.

OK, changed to making it void.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:387
>> +        oldBuffer.swap(m_buffer);
> 
> Why not  `oldBuffer = WTFMove(m_buffer)`?

Nice, fixed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:412
>> +            bucket->copyFrom(*entry);
> 
> If you do this in the above you don't need to do the add twice. You can just use bucket.

Yeah, nice. Fixed.
Comment 19 Yusuke Suzuki 2017-12-12 18:49:06 PST
Committed r225832: <https://trac.webkit.org/changeset/225832>
Comment 20 Radar WebKit Bug Importer 2017-12-12 18:50:39 PST
<rdar://problem/36011741>
Comment 21 Guillaume Emont 2017-12-13 10:18:38 PST
All these weakmap/weakset tests seem to fail on linux testbots. Any idea of what could cause that?
Comment 22 Yusuke Suzuki 2017-12-14 02:46:00 PST
WeakMapGet exhausts the registers. Checking.
Comment 23 Yusuke Suzuki 2017-12-14 03:40:26 PST
(In reply to Yusuke Suzuki from comment #22)
> WeakMapGet exhausts the registers. Checking.

https://bugs.webkit.org/show_bug.cgi?id=180804