| Summary: | AbstractSlotVisitor::containsOpaqueRoot() should only declare didFindOpaqueRoot if the root is actually found. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Mark Lam
2021-05-19 19:24:31 PDT
Created attachment 429126 [details]
proposed patch.
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. (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. 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. Comment on attachment 429126 [details]
proposed patch.
Thanks for the review. Landing now.
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]. |