[webkit-reviews] review granted: [Bug 224715] [JSC] Do not use Bag<> for DFG / FTL watchpoints : [Attachment 426327] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 224715: [JSC] Do not use Bag<> for DFG / FTL watchpoints
https://bugs.webkit.org/show_bug.cgi?id=224715

Attachment 426327: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 426327
  --> https://bugs.webkit.org/attachment.cgi?id=426327
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::AdaptiveInferredPropertyValueWatc
hpointBase()
> +    : 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::AdaptiveInferredPropertyValueWatchpoi
nt()
> +    : 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"?