Bug 226000 – AbstractSlotVisitor::containsOpaqueRoot() should only declare didFindOpaqueRoot if the root is actually found.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

It was erroneously calling didFindOpaqueRoot() all the time even when the root is not found.

rdar://78208014

[Mark Lam]

Created attachment 429126 [details]
proposed patch.

[Robin Morisset]

Comment on attachment 429126 [details]
proposed patch.

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

> Source/JavaScriptCore/heap/AbstractSlotVisitorInlines.h:136
> +    if (UNLIKELY(found && m_needsExtraOpaqueRootHandling)) {

Is there a reason to check found first and m_needsExtraOpaqueRootHandling second?

If anything, I would expect the entire check of m_opaqueRoots.contains(ptr) to be only done after checking m_needsExtraOpaqueRootHandling, especially if it is UNLIKELY.

[Mark Lam]

(In reply to Robin Morisset from comment #2)
> Comment on attachment 429126 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429126&action=review
> 
> > Source/JavaScriptCore/heap/AbstractSlotVisitorInlines.h:136
> > +    if (UNLIKELY(found && m_needsExtraOpaqueRootHandling)) {
> 
> Is there a reason to check found first and m_needsExtraOpaqueRootHandling
> second?
> 
> If anything, I would expect the entire check of m_opaqueRoots.contains(ptr)
> to be only done after checking m_needsExtraOpaqueRootHandling, especially if
> it is UNLIKELY.

I check found first because found is already in memory.  If not found, we save on triggering a load of m_needsExtraOpaqueRootHandling.

[Robin Morisset]

Comment on attachment 429126 [details]
proposed patch.

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

r=me

>>> Source/JavaScriptCore/heap/AbstractSlotVisitorInlines.h:136
>>> +    if (UNLIKELY(found && m_needsExtraOpaqueRootHandling)) {
>> 
>> Is there a reason to check found first and m_needsExtraOpaqueRootHandling second?
>> 
>> If anything, I would expect the entire check of m_opaqueRoots.contains(ptr) to be only done after checking m_needsExtraOpaqueRootHandling, especially if it is UNLIKELY.
> 
> I check found first because found is already in memory.  If not found, we save on triggering a load of m_needsExtraOpaqueRootHandling.

I had missed that we eventually return found and so we need to compute it even if !m_needsExtraOpaqueRootHandling.

[Mark Lam]

Comment on attachment 429126 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

Committed r277773 (237934@main): <https://commits.webkit.org/237934@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429126 [details].