Bug 224715 – [JSC] Do not use Bag<> for DFG / FTL watchpoints

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Do not use Bag<> for DFG / FTL watchpoints

[Yusuke Suzuki]

Created attachment 426324 [details]
Patch

[Yusuke Suzuki]

Created attachment 426325 [details]
Patch

[Yusuke Suzuki]

Created attachment 426326 [details]
Patch

[Yusuke Suzuki]

Created attachment 426327 [details]
Patch

[Darin Adler]

Comment on attachment 426327 [details]
Patch

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

Title should say "for better memory efficiency" or something like that.

While I’m not a deep expert on this code, it looks right to me.

> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:43
> +AdaptiveInferredPropertyValueWatchpointBase::AdaptiveInferredPropertyValueWatchpointBase()
> +    : m_key()
> +{
> +}
> +

Why is this needed? Isn’t this what the default constructor would do if we just wrote "= default"?

> Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.h:45
> +    CodeBlockJettisoningWatchpoint()
> +        : CodeBlockJettisoningWatchpoint(nullptr)
> +    {
> +    }

We could achieve this more economically by just writing "CodeBlock* codeBlock = nullptr" in the other constructor.

> Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.cpp:46
> +AdaptiveInferredPropertyValueWatchpoint::AdaptiveInferredPropertyValueWatchpoint()
> +    : Base()
> +    , m_codeBlock(nullptr)
> +{
> +}

It would be more elegant to initialize m_codeBlock to nullptr in the header, and then use "= default" for this constructor.

> Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:47
> +    , m_codeBlock(nullptr)

Can we initialize m_codeBlock in the class definition, and then omit this line? Maybe not because of JSC_WATCHPOINT_FIELD?

> Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:48
> +    , m_key()

Can we omit this line? Pretty sure that this isn’t needed. Maybe not because of JSC_WATCHPOINT_FIELD?

> Source/JavaScriptCore/dfg/DFGCommonData.cpp:151
> +    for (AdaptiveStructureWatchpoint& watchpoint : m_adaptiveStructureWatchpoints)

I think code like this is more readable with auto& rather than writing out a type name. Using "auto&" documents that the type is correct and does not slice or upcast.

> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:49
> +    WatchpointCollector() = default;

I believe we can simply omit this entirely, and behavior should be the same.

> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:60
> +    template<typename Func>
> +    void addWatchpoint(const Func& func)

Could we use a word instead of "func"?

[Yusuke Suzuki]

Comment on attachment 426327 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:43
>> +
> 
> Why is this needed? Isn’t this what the default constructor would do if we just wrote "= default"?

Yeah, replaced with = default;

>> Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.h:45
>> +    }
> 
> We could achieve this more economically by just writing "CodeBlock* codeBlock = nullptr" in the other constructor.

Replaced.

>> Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.cpp:46
>> +}
> 
> It would be more elegant to initialize m_codeBlock to nullptr in the header, and then use "= default" for this constructor.

Replaced.

>> Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:47
>> +    , m_codeBlock(nullptr)
> 
> Can we initialize m_codeBlock in the class definition, and then omit this line? Maybe not because of JSC_WATCHPOINT_FIELD?

Unfortunately it is not since JSC_WATCHPOINT_FIELD.

>> Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:48
>> +    , m_key()
> 
> Can we omit this line? Pretty sure that this isn’t needed. Maybe not because of JSC_WATCHPOINT_FIELD?

Yes, we can omit it.

>> Source/JavaScriptCore/dfg/DFGCommonData.cpp:151
>> +    for (AdaptiveStructureWatchpoint& watchpoint : m_adaptiveStructureWatchpoints)
> 
> I think code like this is more readable with auto& rather than writing out a type name. Using "auto&" documents that the type is correct and does not slice or upcast.

Replaced.

>> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:49
>> +    WatchpointCollector() = default;
> 
> I believe we can simply omit this entirely, and behavior should be the same.

Removed.

>> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:60
>> +    void addWatchpoint(const Func& func)
> 
> Could we use a word instead of "func"?

Replaced with function.

[Yusuke Suzuki]

Committed r276226 (236708@main): <https://commits.webkit.org/236708@main>

[Yusuke Suzuki]

Comment on attachment 426327 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:49
>>> +    WatchpointCollector() = default;
>> 
>> I believe we can simply omit this entirely, and behavior should be the same.
> 
> Removed.

Ah, this was important. If we are using WTF_MAKE_NONCOPYABLE, we need this. Otherwise, this becomes compile error.

[Yusuke Suzuki]

Committed r276227 (236709@main): <https://commits.webkit.org/236709@main>

[Ryan Haddad]

rdar://76810095