Bug 207715 – [JSC] JITThunk should be HashSet<Weak<NativeExecutable>> with appropriate GC weakness handling

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Radar WebKit Bug Importer]

<rdar://problem/59471675>

[Yusuke Suzuki]

Created attachment 390848 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 390849 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 390852 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 390858 [details]
Patch

[Darin Adler]

Comment on attachment 390858 [details]
Patch

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

Did not do review+ yet because of the EWS failure

> Source/JavaScriptCore/heap/Weak.h:52
> +    inline Weak(T*, WeakHandleOwner* = nullptr, void* context = nullptr);

What value does the “inline” keyword here have? Couldn’t we omit it without changing anything?

> Source/JavaScriptCore/heap/Weak.h:55
> +    inline Weak(WTF::HashTableDeletedValueType);

Ditto.

> Source/JavaScriptCore/heap/Weak.h:56
> +    inline bool isHashTableEmptyValue() const;

Ditto.

> Source/JavaScriptCore/heap/Weak.h:79
> +    inline WeakImpl* unsafeImpl() const { return m_impl; }

Ditto.

> Source/JavaScriptCore/heap/WeakInlines.h:44
> +template<typename T> inline bool Weak<T>::isHashTableEmptyValue() const

Can this be constexpr instead of just inline? But also, why do we need the inline keyword? As I understand it inline primarily relaxes the one definition rule so we can include things in headers. Templates already have that relaxed so I don’t think we need to say “inline”.

> Source/JavaScriptCore/heap/WeakInlines.h:49
> +template<typename T> inline Weak<T>::Weak(WTF::HashTableDeletedValueType)

Same questions. Can we use constexpr instead? Can we omit inline?

> Source/JavaScriptCore/jit/JITThunks.cpp:47
> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const NativeExecutable* executable)

Seems unnecessary to say “inline” on this function (here or in the header). Whether it’s compile in line or not is likely unaffected by that.

> Source/JavaScriptCore/jit/JITThunks.cpp:52
> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const Weak<NativeExecutable>& key)

Ditto.

> Source/JavaScriptCore/jit/JITThunks.cpp:66
> +inline bool JITThunks::WeakNativeExecutableHash::equal(const Weak<NativeExecutable>& a, const Weak<NativeExecutable>& b)

Ditto.

> Source/JavaScriptCore/jit/JITThunks.cpp:70
> +    auto* bImpl = b.unsafeImpl();

Can we just do the bImpl assertions and call the equal function that takes a raw pointer? Would be nice to have less repetitive code. Maybe even write a function that gets the executable pointer and does the assertions so we don’t have to repeat those.

> Source/JavaScriptCore/jit/JITThunks.cpp:82
> +inline bool JITThunks::WeakNativeExecutableHash::equal(const Weak<NativeExecutable>& a, const NativeExecutable* bExecutable)

Ditto.

> Source/JavaScriptCore/jit/JITThunks.cpp:181
> +        auto iterator = m_nativeExecutableSet.find<HostKeySearcher>(hostFunctionKey);

Seems like we should be using remove directly instead of find/remove. I suppose using find/remove lets us write assertions.

> Source/JavaScriptCore/jit/JITThunks.cpp:202
> +        auto iterator = m_nativeExecutableSet.find<HostKeySearcher>(hostFunctionKey);

Can we write this with HashSet::get instead of find/end/iterator->get()?

> Source/JavaScriptCore/jit/JITThunks.cpp:227
> +            {

I don’t think we need braces here.

> Source/JavaScriptCore/jit/JITThunks.cpp:228
> +                // Override the existing Weak<NativeExecutable> with the new one since it is dead.

Usually this kind of operation is called set<> but maybe we don’t have a suitable one for Weak?

> Source/JavaScriptCore/jit/JITThunks.cpp:234
> +            {

I don’t think we need braces here.

> Source/JavaScriptCore/jit/JITThunks.h:87
> +        static bool equal(const Weak<NativeExecutable>&, const NativeExecutable*);

The const here is different from the other const. Not clear why the NativeExecutable is const in this case, but not in the others. Consider that “const NativeExecutable*” is like “Weak<const NativeExecutable>”.

> Source/JavaScriptCore/jit/JITThunks.h:98
> +            unsigned hash = WTF::pairIntHash(hashPointer(function), hashPointer(constructor));

All this hashing things that are already hashed seems unfortunate. I think it’s a better pattern to use Hasher the way we would use a StringBuilder. That’s what the computeHash function template is for. Something like this should work:

    return computeHash(function, constructor, name);

It it doesn’t work would be nice to fix it.

[Yusuke Suzuki]

Comment on attachment 390858 [details]
Patch

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

EWS failure is due to the different Wasm patch.

>> Source/JavaScriptCore/heap/Weak.h:52
>> +    inline Weak(T*, WeakHandleOwner* = nullptr, void* context = nullptr);
> 
> What value does the “inline” keyword here have? Couldn’t we omit it without changing anything?

We can omit it. Fixed.

>> Source/JavaScriptCore/heap/Weak.h:55
>> +    inline Weak(WTF::HashTableDeletedValueType);
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/heap/Weak.h:56
>> +    inline bool isHashTableEmptyValue() const;
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/heap/Weak.h:79
>> +    inline WeakImpl* unsafeImpl() const { return m_impl; }
> 
> Ditto.

Ditto

>> Source/JavaScriptCore/heap/WeakInlines.h:44
>> +template<typename T> inline bool Weak<T>::isHashTableEmptyValue() const
> 
> Can this be constexpr instead of just inline? But also, why do we need the inline keyword? As I understand it inline primarily relaxes the one definition rule so we can include things in headers. Templates already have that relaxed so I don’t think we need to say “inline”.

I think we can omit it. This “inline” exists just because it was here before this patch.

>> Source/JavaScriptCore/heap/WeakInlines.h:49
>> +template<typename T> inline Weak<T>::Weak(WTF::HashTableDeletedValueType)
> 
> Same questions. Can we use constexpr instead? Can we omit inline?

Yes, fixed.

>> Source/JavaScriptCore/jit/JITThunks.cpp:47
>> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const NativeExecutable* executable)
> 
> Seems unnecessary to say “inline” on this function (here or in the header). Whether it’s compile in line or not is likely unaffected by that.

I would like to put here since this can ensure that (1) it is inlined, and (2) it is ensured that this function in only called from this cpp file.

>> Source/JavaScriptCore/jit/JITThunks.cpp:52
>> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const Weak<NativeExecutable>& key)
> 
> Ditto.

Ditto here.

>> Source/JavaScriptCore/jit/JITThunks.cpp:66
>> +inline bool JITThunks::WeakNativeExecutableHash::equal(const Weak<NativeExecutable>& a, const Weak<NativeExecutable>& b)
> 
> Ditto.

Ditto here.

>> Source/JavaScriptCore/jit/JITThunks.cpp:70
>> +    auto* bImpl = b.unsafeImpl();
> 
> Can we just do the bImpl assertions and call the equal function that takes a raw pointer? Would be nice to have less repetitive code. Maybe even write a function that gets the executable pointer and does the assertions so we don’t have to repeat those.

Fixed.

>> Source/JavaScriptCore/jit/JITThunks.cpp:82
>> +inline bool JITThunks::WeakNativeExecutableHash::equal(const Weak<NativeExecutable>& a, const NativeExecutable* bExecutable)
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/jit/JITThunks.cpp:181
>> +        auto iterator = m_nativeExecutableSet.find<HostKeySearcher>(hostFunctionKey);
> 
> Seems like we should be using remove directly instead of find/remove. I suppose using find/remove lets us write assertions.

We currently do no support remove using HashTranslator. In this patch, I’ll just use find and remove which works without introducing inefficiency.

>> Source/JavaScriptCore/jit/JITThunks.cpp:202
>> +        auto iterator = m_nativeExecutableSet.find<HostKeySearcher>(hostFunctionKey);
> 
> Can we write this with HashSet::get instead of find/end/iterator->get()?

We currently do not support get w/ HashTranslator, so this patch just uses find.

>> Source/JavaScriptCore/jit/JITThunks.cpp:227
>> +            {
> 
> I don’t think we need braces here.

It is just added to perform sanity check after cleaning up all iterators etc. OK for removing.

>> Source/JavaScriptCore/jit/JITThunks.cpp:228
>> +                // Override the existing Weak<NativeExecutable> with the new one since it is dead.
> 
> Usually this kind of operation is called set<> but maybe we don’t have a suitable one for Weak?

HashSet does not have set currently. We could add it but I would like to put this as a FIXME since this is not related to JITThunks’ change itself.

>> Source/JavaScriptCore/jit/JITThunks.cpp:234
>> +            {
> 
> I don’t think we need braces here.

Ditto.

>> Source/JavaScriptCore/jit/JITThunks.h:87
>> +        static bool equal(const Weak<NativeExecutable>&, const NativeExecutable*);
> 
> The const here is different from the other const. Not clear why the NativeExecutable is const in this case, but not in the others. Consider that “const NativeExecutable*” is like “Weak<const NativeExecutable>”.

Just removing const here is OK. I think we do not have an implementation of Weak<const NativeExecutable> and maybe adding it is not so profitable.

>> Source/JavaScriptCore/jit/JITThunks.h:98
>> +            unsigned hash = WTF::pairIntHash(hashPointer(function), hashPointer(constructor));
> 
> All this hashing things that are already hashed seems unfortunate. I think it’s a better pattern to use Hasher the way we would use a StringBuilder. That’s what the computeHash function template is for. Something like this should work:
> 
>     return computeHash(function, constructor, name);
> 
> It it doesn’t work would be nice to fix it.

Checking, if it does not work, I’ll put FIXME for now.

[Yusuke Suzuki]

Comment on attachment 390858 [details]
Patch

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

>>> Source/JavaScriptCore/heap/WeakInlines.h:49
>>> +template<typename T> inline Weak<T>::Weak(WTF::HashTableDeletedValueType)
>> 
>> Same questions. Can we use constexpr instead? Can we omit inline?
> 
> Yes, fixed.

Unfortunately, it is not possible because we cannot produce hashTableDeletedValue without using reinterpret_cast, which is not constexpr.

>>> Source/JavaScriptCore/jit/JITThunks.h:98
>>> +            unsigned hash = WTF::pairIntHash(hashPointer(function), hashPointer(constructor));
>> 
>> All this hashing things that are already hashed seems unfortunate. I think it’s a better pattern to use Hasher the way we would use a StringBuilder. That’s what the computeHash function template is for. Something like this should work:
>> 
>>     return computeHash(function, constructor, name);
>> 
>> It it doesn’t work would be nice to fix it.
> 
> Checking, if it does not work, I’ll put FIXME for now.

Didn't work. putting FIXME here.

[Yusuke Suzuki]

Created attachment 390908 [details]
Patch

[Yusuke Suzuki]

Created attachment 390910 [details]
Patch

[Darin Adler]

Comment on attachment 390858 [details]
Patch

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

Thanks for your thoughtful consideration of all my comments.

>>> Source/JavaScriptCore/jit/JITThunks.cpp:47
>>> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const NativeExecutable* executable)
>> 
>> Seems unnecessary to say “inline” on this function (here or in the header). Whether it’s compile in line or not is likely unaffected by that.
> 
> I would like to put here since this can ensure that (1) it is inlined, and (2) it is ensured that this function in only called from this cpp file.

Sounds OK. Does writing "inline" ensure (1) and (2)? For (1), if it’s not inlined, how do we find out? I’m quite sure that writing "inline" does not guarantee something is inlined. For (2), I guess if it it’s called from outside the file, maybe we find out because of a linker error in release builds?

>>> Source/JavaScriptCore/jit/JITThunks.cpp:227
>>> +            {
>> 
>> I don’t think we need braces here.
> 
> It is just added to perform sanity check after cleaning up all iterators etc. OK for removing.

I don’t understand what you mean. Sanity check of what?

>>> Source/JavaScriptCore/jit/JITThunks.cpp:234
>>> +            {
>> 
>> I don’t think we need braces here.
> 
> Ditto.

I think these braces have no effect. I don’t understand what you mean.

>>>> Source/JavaScriptCore/jit/JITThunks.h:98
>>>> +            unsigned hash = WTF::pairIntHash(hashPointer(function), hashPointer(constructor));
>>> 
>>> All this hashing things that are already hashed seems unfortunate. I think it’s a better pattern to use Hasher the way we would use a StringBuilder. That’s what the computeHash function template is for. Something like this should work:
>>> 
>>>     return computeHash(function, constructor, name);
>>> 
>>> It it doesn’t work would be nice to fix it.
>> 
>> Checking, if it does not work, I’ll put FIXME for now.
> 
> Didn't work. putting FIXME here.

I’d love to fix Hasher so it can handle this case. Glad you added the FIXME.

[Mark Lam]

(In reply to Darin Adler from comment #11)
> Comment on attachment 390858 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390858&action=review
> 
> Thanks for your thoughtful consideration of all my comments.
> 
> >>> Source/JavaScriptCore/jit/JITThunks.cpp:47
> >>> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const NativeExecutable* executable)
> >> 
> >> Seems unnecessary to say “inline” on this function (here or in the header). Whether it’s compile in line or not is likely unaffected by that.
> > 
> > I would like to put here since this can ensure that (1) it is inlined, and (2) it is ensured that this function in only called from this cpp file.
> 
> Sounds OK. Does writing "inline" ensure (1) and (2)? For (1), if it’s not
> inlined, how do we find out? I’m quite sure that writing "inline" does not
> guarantee something is inlined. For (2), I guess if it it’s called from
> outside the file, maybe we find out because of a linker error in release
> builds?

With the inline modifier, if the inline implementation is not present in the current compilation unit, we'll get a compile time error.  Without the inline modifier, we'll have to wait till link time to find out, and only on a non-debug build.

[Darin Adler]

Comment on attachment 390910 [details]
Patch

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

> Source/JavaScriptCore/jit/JITThunks.cpp:69
> +    if (&a == &b)
> +        return true;

Is this an important optimization?

> Source/JavaScriptCore/jit/JITThunks.cpp:91
> +    auto* aImpl = a.unsafeImpl();
> +    ASSERT(aImpl);
> +    ASSERT(aImpl->state() != WeakImpl::State::Deallocated);
> +    return equal(*static_cast<NativeExecutable*>(aImpl->jsValue().asCell()), *bExecutable);

I think that this sequence should be a helper function. Going from a Weak<NativeExecutable> to a NativeExecutable& with all the assertions. Instead it’s repeated three times. Twice above and once here.

[Darin Adler]

(In reply to Mark Lam from comment #12)
> With the inline modifier, if the inline implementation is not present in the
> current compilation unit, we'll get a compile time error.  Without the
> inline modifier, we'll have to wait till link time to find out, and only on
> a non-debug build.

You are talking about the inline modifier inside the class definition. That’s a good point for cases like this. Ones where we intentionally have the inline in the .cpp file and want to make sure no one accidentally uses it outside the .cpp file. That’s one case where I could imagine using it the class definition.

Here we were discussing the use of the inline modifier in front of the function definition in the .cpp file, which is a different topic.

[Yusuke Suzuki]

Comment on attachment 390858 [details]
Patch

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

>>>> Source/JavaScriptCore/jit/JITThunks.cpp:227
>>>> +            {
>>> 
>>> I don’t think we need braces here.
>> 
>> It is just added to perform sanity check after cleaning up all iterators etc. OK for removing.
> 
> I don’t understand what you mean. Sanity check of what?

The code rounded by `#if ASSERT_ENABLED` is sanity-checking code. My original intention was, I would like to perform this sanity-checking code after cleaning up all the use of this hashset including iterators.
That's why I had braces here.

[Darin Adler]

(In reply to Yusuke Suzuki from comment #15)
> The code rounded by `#if ASSERT_ENABLED` is sanity-checking code. My
> original intention was, I would like to perform this sanity-checking code
> after cleaning up all the use of this hashset including iterators.
> That's why I had braces here.

What’s an example of an iterator that needs to be cleaned up? Is there one in a local variable?

[Mark Lam]

(In reply to Darin Adler from comment #14)
> (In reply to Mark Lam from comment #12)
> > With the inline modifier, if the inline implementation is not present in the
> > current compilation unit, we'll get a compile time error.  Without the
> > inline modifier, we'll have to wait till link time to find out, and only on
> > a non-debug build.
> 
> You are talking about the inline modifier inside the class definition.
> That’s a good point for cases like this. Ones where we intentionally have
> the inline in the .cpp file and want to make sure no one accidentally uses
> it outside the .cpp file. That’s one case where I could imagine using it the
> class definition.
> 
> Here we were discussing the use of the inline modifier in front of the
> function definition in the .cpp file, which is a different topic.

Oops, you're right.  Sorry for the noise.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #16)
> (In reply to Yusuke Suzuki from comment #15)
> > The code rounded by `#if ASSERT_ENABLED` is sanity-checking code. My
> > original intention was, I would like to perform this sanity-checking code
> > after cleaning up all the use of this hashset including iterators.
> > That's why I had braces here.
> 
> What’s an example of an iterator that needs to be cleaned up? Is there one
> in a local variable?

The iterator can affect on HashSet's invalidated-iterator tracking code.
In this code's use case, this does not matter. But when performing sanity-checking code, I personally like to perform it in an isolated way, which cleans up all the related data to the sanity-checked data-structure :)

[Yusuke Suzuki]

Comment on attachment 390910 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITThunks.cpp:69
>> +        return true;
> 
> Is this an important optimization?

Yes, this is important. This path should be encouraged when rehashing happens in HashSet.

>> Source/JavaScriptCore/jit/JITThunks.cpp:91
>> +    return equal(*static_cast<NativeExecutable*>(aImpl->jsValue().asCell()), *bExecutable);
> 
> I think that this sequence should be a helper function. Going from a Weak<NativeExecutable> to a NativeExecutable& with all the assertions. Instead it’s repeated three times. Twice above and once here.

OK, I see. Fixed.

[Yusuke Suzuki]

Comment on attachment 390858 [details]
Patch

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

>>>>> Source/JavaScriptCore/jit/JITThunks.cpp:47
>>>>> +inline unsigned JITThunks::WeakNativeExecutableHash::hash(const NativeExecutable* executable)
>>>> 
>>>> Seems unnecessary to say “inline” on this function (here or in the header). Whether it’s compile in line or not is likely unaffected by that.
>>> 
>>> I would like to put here since this can ensure that (1) it is inlined, and (2) it is ensured that this function in only called from this cpp file.
>> 
>> Sounds OK. Does writing "inline" ensure (1) and (2)? For (1), if it’s not inlined, how do we find out? I’m quite sure that writing "inline" does not guarantee something is inlined. For (2), I guess if it it’s called from outside the file, maybe we find out because of a linker error in release builds?
> 
> With the inline modifier, if the inline implementation is not present in the current compilation unit, we'll get a compile time error.  Without the inline modifier, we'll have to wait till link time to find out, and only on a non-debug build.

This inline case, I intend to enforce (2) thing by putting "inline" in cpp file side. Putting "inline" in function declarations in class / function definition in cpp file is the same. And I think we tend to put "inline" in definition instead of declaration :)

[Yusuke Suzuki]

Committed r256779: <https://trac.webkit.org/changeset/256779>

[Brent Fulgham]

*** Bug 186736 has been marked as a duplicate of this bug. ***