Bug 227677 – [JSC] Optimize Object.assign and putDirectInternal

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

[Yusuke Suzuki]

[JSC] Optimize Object.assign and putDirectInternal

[Yusuke Suzuki]

Created attachment 432871 [details]
Patch

[Yusuke Suzuki]

Created attachment 432872 [details]
Patch

[Yusuke Suzuki]

Created attachment 432873 [details]
Patch

[Filip Pizlo]

Comment on attachment 432873 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2941
> +            // ToObject is idempotent if it succeeds. Plus, it is non-observable except for the case that an exception is thrown. And when the exception is thrown,
> +            // we exit from DFG / FTL. Plus, we keep ordering of these two ToObject because clobberizing rule says clobberTop. So,
> +            // we can say exitOK for each ToObject.

If it's only observable when an exception is thrown, then why do you need to put in ExitOK?

[Yusuke Suzuki]

Comment on attachment 432873 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2941
>> +            // we can say exitOK for each ToObject.
> 
> If it's only observable when an exception is thrown, then why do you need to put in ExitOK?

The reason is that, even for exception-exit, we make it ExitInvalid. In fix-up phase, we convert them to `Identify(Check:Object)` in many cases, and then we failed to insert these checks since the state is ExitInvalid.
We need to ensure that state is ExitOK after these ToObject.

[Yusuke Suzuki]

Committed r279604 (239428@main): <https://commits.webkit.org/239428@main>

[Radar WebKit Bug Importer]

<rdar://problem/80225148>

[Darin Adler]

Comment on attachment 432873 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:17
> +            2.2. Before r277620, we are checking m_replacementWatchpointSets's nullptr and that was fast. But after that, we are always
> +                 calling HashMap::get, and it is not inlined. This means that if we have StructureRareData, we are always calling HashMap::get
> +                 even though there is no m_replacementWatchpointSets set. This patch adds HashMap::isNullStorage to avoid this call by inlinely
> +                 check this via `LIKELY(m_replacementWatchpointSets.isNullStorage())`.

It seems like this is not a unique case. Should we instead change HashMap::get so that the empty case is always inlined and marked LIKELY? Or add an overload of HashMap::get? Not wanting to block landing this; seems like a more-refined way to handle this that can help many other call sites.

[Yusuke Suzuki]

Comment on attachment 432873 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:17
>> +                 check this via `LIKELY(m_replacementWatchpointSets.isNullStorage())`.
> 
> It seems like this is not a unique case. Should we instead change HashMap::get so that the empty case is always inlined and marked LIKELY? Or add an overload of HashMap::get? Not wanting to block landing this; seems like a more-refined way to handle this that can help many other call sites.

I think whether nullptr is LIKELY or not highly depends on the places. I think many HashMaps will have at least one entry and in that case, this LIKELY will rather hurt the performance.
But in this place, nullptr is highly expected, and that's why I inserted this branch for this place.
I think it is OK to inline `if (m_table)` check in HashMap::get globally, but I don't think putting `LIKELY` there is good.

[Darin Adler]

Comment on attachment 432873 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:17
>>> +                 check this via `LIKELY(m_replacementWatchpointSets.isNullStorage())`.
>> 
>> It seems like this is not a unique case. Should we instead change HashMap::get so that the empty case is always inlined and marked LIKELY? Or add an overload of HashMap::get? Not wanting to block landing this; seems like a more-refined way to handle this that can help many other call sites.
> 
> I think whether nullptr is LIKELY or not highly depends on the places. I think many HashMaps will have at least one entry and in that case, this LIKELY will rather hurt the performance.
> But in this place, nullptr is highly expected, and that's why I inserted this branch for this place.
> I think it is OK to inline `if (m_table)` check in HashMap::get globally, but I don't think putting `LIKELY` there is good.

Sounds fine.

I am pretty sure that there are a lot of "almost always empty" maps. I agree that there are also many "likely to have at least one thing" maps. In the end we just have to measure like we always do. Could have multiple versions of get and find if this really has to be carefully optimized.