Bug 143919 – Merge WeakMapData into JSWeakMap and JSWeakSet

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Integrate WeakMapData into WeakMap / WeakSet as inlined member

[Yusuke Suzuki]

Created attachment 318955 [details]
Patch

[Yusuke Suzuki]

Created attachment 318958 [details]
Patch

[Yusuke Suzuki]

Created attachment 318969 [details]
Patch

[Darin Adler]

Comment on attachment 318969 [details]
Patch

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

I’m not a big fan of the abbreviation “Impl” as in “WeakMapImpl”; I would have used the word “Base” instead and called it “WeakMapBase”, I think.

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:47
> +void WeakMapImpl::finishCreation(VM& vm)
> +{
> +    Base::finishCreation(vm);
> +}

Do we really need this function?

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:56
> +    WeakMapImpl* thisObj = jsCast<WeakMapImpl*>(cell);

I would use auto* here since the type name is already there. I also would use the word “object”, not the abbreviation “obj”. But these are changes to code that was already here, I guess. And of course I would use a reference instead of a pointer, but I think the people who normally work on JavaScriptCore strongly reject this preference of mine.

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:63
> +    WeakMapImpl* thisObj = jsCast<WeakMapImpl*>(cell);

Ditto.

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:95
> +    auto iter = m_map.find(key);
> +    if (iter == m_map.end())
> +        return false;
> +
> +    m_map.remove(iter);
> +    return true;

This entire sequence of code does the same thing as:

    return m_map.remove(key);

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:118
> +    RELEASE_ASSERT(m_liveKeyCount <= impl->m_map.size());

Very strange to use RELEASE_ASSERT here. There is no chance this assertion will fail, given the way the loop above is written, so why include the code in release builds? A normal ASSERT makes way more sense to me.

> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:123
> +    WeakMapImpl* impl = target();

I would call this map, or weakMap, not impl. Words preferred over abbreviations.

> Source/JavaScriptCore/runtime/WeakMapImpl.h:47
> +    typedef HashMap<JSObject*, WriteBarrier<Unknown>> MapType;

In new code we should us "using" rather than "typedef".

> Source/JavaScriptCore/runtime/WeakMapImpl.h:51
> +    int size() const { return m_map.size(); }

The choice of "int" for the type here is peculiar. HashMap uses "unsigned".

> Source/JavaScriptCore/runtime/WeakMapImpl.h:58
> +    void finishCreation(VM&);

Strange to override the public function in the base class with a protected function. Maybe we should be inheriting protected instead of public?

> Source/JavaScriptCore/runtime/WeakMapImpl.h:65
> +        DeadKeyCleaner(WeakMapImpl* impl)
> +        {
> +            ASSERT_UNUSED(impl, impl == target());
> +        }

I think we should just assert this in the WeakMapImpl constructor after constructing the DeadKeyCleaner. Seems like overkill to write this constructor here in this header file just so we can put the assertion here.

> Source/JavaScriptCore/runtime/WeakMapImpl.h:70
> +        WeakMapImpl* target()
> +        {
> +            return bitwise_cast<WeakMapImpl*>(bitwise_cast<char*>(this) - OBJECT_OFFSETOF(WeakMapImpl, m_deadKeyCleaner));
> +        }

I suggest putting this function body inside the cpp file rather than here in the header. It can still be marked inline, but since it will only be called inside the cpp file we should not have to put it here. We can still define the function here.

[Darin Adler]

Comment on attachment 318969 [details]
Patch

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

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:70
>> +        }
> 
> I suggest putting this function body inside the cpp file rather than here in the header. It can still be marked inline, but since it will only be called inside the cpp file we should not have to put it here. We can still define the function here.

We can still *declare* the function here, I mean.

[Yusuke Suzuki]

Committed r221223: <http://trac.webkit.org/changeset/221223>

[Radar WebKit Bug Importer]

<rdar://problem/34096516>

[Yusuke Suzuki]

Comment on attachment 318969 [details]
Patch

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

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:47
>> +}
> 
> Do we really need this function?

OK, dropped.

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:56
>> +    WeakMapImpl* thisObj = jsCast<WeakMapImpl*>(cell);
> 
> I would use auto* here since the type name is already there. I also would use the word “object”, not the abbreviation “obj”. But these are changes to code that was already here, I guess. And of course I would use a reference instead of a pointer, but I think the people who normally work on JavaScriptCore strongly reject this preference of mine.

OK, here, jsCast<> explicitly describes the type. So auto* would be nice. Changed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:63
>> +    WeakMapImpl* thisObj = jsCast<WeakMapImpl*>(cell);
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:95
>> +    return true;
> 
> This entire sequence of code does the same thing as:
> 
>     return m_map.remove(key);

Nice. Right. Fixed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:118
>> +    RELEASE_ASSERT(m_liveKeyCount <= impl->m_map.size());
> 
> Very strange to use RELEASE_ASSERT here. There is no chance this assertion will fail, given the way the loop above is written, so why include the code in release builds? A normal ASSERT makes way more sense to me.

Make sense. Changed to ASSERT.

>> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:123
>> +    WeakMapImpl* impl = target();
> 
> I would call this map, or weakMap, not impl. Words preferred over abbreviations.

Changed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:47
>> +    typedef HashMap<JSObject*, WriteBarrier<Unknown>> MapType;
> 
> In new code we should us "using" rather than "typedef".

Changed.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:51
>> +    int size() const { return m_map.size(); }
> 
> The choice of "int" for the type here is peculiar. HashMap uses "unsigned".

Changed it to unsigned.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:58
>> +    void finishCreation(VM&);
> 
> Strange to override the public function in the base class with a protected function. Maybe we should be inheriting protected instead of public?

OK, aligned them to the base class's policy. destroy is protected. estimatedSize and visitChildren are public.

>> Source/JavaScriptCore/runtime/WeakMapImpl.h:65
>> +        }
> 
> I think we should just assert this in the WeakMapImpl constructor after constructing the DeadKeyCleaner. Seems like overkill to write this constructor here in this header file just so we can put the assertion here.

Sounds nice. Fixed.

>>> Source/JavaScriptCore/runtime/WeakMapImpl.h:70
>>> +        }
>> 
>> I suggest putting this function body inside the cpp file rather than here in the header. It can still be marked inline, but since it will only be called inside the cpp file we should not have to put it here. We can still define the function here.
> 
> We can still *declare* the function here, I mean.

Sounds very nice. Fixed.