Bug 143919 - Merge WeakMapData into JSWeakMap and JSWeakSet
Summary: Merge WeakMapData into JSWeakMap and JSWeakSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 142408
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-18 11:50 PDT by Yusuke Suzuki
Modified: 2017-08-26 01:03 PDT (History)
7 users (show)

See Also:


Attachments
Patch (41.25 KB, patch)
2017-08-23 19:26 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (41.22 KB, patch)
2017-08-23 19:30 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (41.22 KB, patch)
2017-08-23 22:17 PDT, Yusuke Suzuki
darin: 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 2015-04-18 11:50:22 PDT
Integrate WeakMapData into WeakMap / WeakSet as inlined member
Comment 1 Yusuke Suzuki 2017-08-23 19:26:45 PDT
Created attachment 318955 [details]
Patch
Comment 2 Yusuke Suzuki 2017-08-23 19:30:09 PDT
Created attachment 318958 [details]
Patch
Comment 3 Yusuke Suzuki 2017-08-23 22:17:31 PDT
Created attachment 318969 [details]
Patch
Comment 4 Darin Adler 2017-08-25 20:42:35 PDT
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.
Comment 5 Darin Adler 2017-08-25 21:12:14 PDT
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.
Comment 6 Yusuke Suzuki 2017-08-26 01:01:36 PDT
Committed r221223: <http://trac.webkit.org/changeset/221223>
Comment 7 Radar WebKit Bug Importer 2017-08-26 01:02:43 PDT
<rdar://problem/34096516>
Comment 8 Yusuke Suzuki 2017-08-26 01:03:17 PDT
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.