226000 2021-05-19 19:24:31 -0700 AbstractSlotVisitor::containsOpaqueRoot() should only declare didFindOpaqueRoot if the root is actually found. 2021-05-19 20:34:40 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam ews-watchlist keith_miller msaboff rmorisset saam tzagallo webkit-bug-importer oldest_to_newest 1761809 0 mark.lam 2021-05-19 19:24:31 -0700 It was erroneously calling didFindOpaqueRoot() all the time even when the root is not found. rdar://78208014 1761812 1 429126 mark.lam 2021-05-19 19:29:55 -0700 Created attachment 429126 proposed patch. 1761814 2 429126 rmorisset 2021-05-19 19:35:25 -0700 Comment on attachment 429126 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. 1761815 3 mark.lam 2021-05-19 19:37:52 -0700 (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. 1761817 4 429126 rmorisset 2021-05-19 19:43:14 -0700 Comment on attachment 429126 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. 1761818 5 429126 mark.lam 2021-05-19 19:45:22 -0700 Comment on attachment 429126 proposed patch. Thanks for the review. Landing now. 1761823 6 ews-feeder 2021-05-19 20:34:36 -0700 Committed r277773 (237934@main): <https://commits.webkit.org/237934@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429126. 429126 2021-05-19 19:29:55 -0700 2021-05-19 20:34:38 -0700 proposed patch. bug-226000.patch text/plain 1678 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjc3NzcxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDIxLTA1LTE5ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBB YnN0cmFjdFNsb3RWaXNpdG9yOjpjb250YWluc09wYXF1ZVJvb3QoKSBzaG91bGQgb25seSBkZWNs YXJlIGRpZEZpbmRPcGFxdWVSb290IGlmIHRoZSByb290IGlzIGFjdHVhbGx5IGZvdW5kLgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI2MDAwCisgICAg ICAgIHJkYXI6Ly83ODIwODAxNAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIEl0IHdhcyBlcnJvbmVvdXNseSBjYWxsaW5nIGRpZEZpbmRPcGFxdWVSb290 KCkgYWxsIHRoZSB0aW1lIGV2ZW4gd2hlbiB0aGUgcm9vdCBpcworICAgICAgICBub3QgZm91bmQu ICBUaGlzIG9ubHkgYWZmZWN0cyB0aGUgR0MgdmVyaWZpZXIgdG9vbC4gIEl0IGhhcyBubyBlZmZl Y3Qgb24gcHJvZHVjdGlvbgorICAgICAgICBjb2RlLgorCisgICAgICAgICogaGVhcC9BYnN0cmFj dFNsb3RWaXNpdG9ySW5saW5lcy5oOgorICAgICAgICAoSlNDOjpBYnN0cmFjdFNsb3RWaXNpdG9y Ojpjb250YWluc09wYXF1ZVJvb3QgY29uc3QpOgorCiAyMDIxLTA1LTE5ICBNYXJrIExhbSAgPG1h cmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBTcGVlZCB1cCBkZWJ1ZyB2YWxpZGF0aW9uIGlu IENhY2hlYWJsZUlkZW50aWZpZXI6OmNyZWF0ZUZyb21JZGVudGlmaWVyT3duZWRCeUNvZGVCbG9j aygpLgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvQWJzdHJhY3RTbG90VmlzaXRv cklubGluZXMuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9BYnN0 cmFjdFNsb3RWaXNpdG9ySW5saW5lcy5oCShyZXZpc2lvbiAyNzc3NTgpCisrKyBTb3VyY2UvSmF2 YVNjcmlwdENvcmUvaGVhcC9BYnN0cmFjdFNsb3RWaXNpdG9ySW5saW5lcy5oCSh3b3JraW5nIGNv cHkpCkBAIC0xMzMsNyArMTMzLDcgQEAgaW5saW5lIGJvb2wgQWJzdHJhY3RTbG90VmlzaXRvcjo6 YWRkT3BhcQogaW5saW5lIGJvb2wgQWJzdHJhY3RTbG90VmlzaXRvcjo6Y29udGFpbnNPcGFxdWVS b290KHZvaWQqIHB0cikgY29uc3QKIHsKICAgICBib29sIGZvdW5kID0gbV9vcGFxdWVSb290cy5j b250YWlucyhwdHIpOwotICAgIGlmIChVTkxJS0VMWShtX25lZWRzRXh0cmFPcGFxdWVSb290SGFu ZGxpbmcpKSB7CisgICAgaWYgKFVOTElLRUxZKGZvdW5kICYmIG1fbmVlZHNFeHRyYU9wYXF1ZVJv b3RIYW5kbGluZykpIHsKICAgICAgICAgYXV0byogbm9uQ29uc3RUaGlzID0gY29uc3RfY2FzdDxB YnN0cmFjdFNsb3RWaXNpdG9yKj4odGhpcyk7CiAgICAgICAgIG5vbkNvbnN0VGhpcy0+ZGlkRmlu ZE9wYXF1ZVJvb3QocHRyKTsKICAgICB9Cg==