Bug 236387

Summary: Use local variable pointer for concurrently load value
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch saam: review+

Yusuke Suzuki
Reported 2022-02-09 11:28:35 PST
Use WTF::opaque in some places to ensure access is done only once
Attachments
Patch (15.05 KB, patch)
2022-02-09 11:33 PST, Yusuke Suzuki 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (15.16 KB, patch)
2022-02-09 13:32 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (14.86 KB, patch)
2022-02-09 15:03 PST, Yusuke Suzuki 		saam: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2022-02-09 11:33:01 PST
Created attachment 451418 [details] Patch
Yusuke Suzuki
Comment 2 2022-02-09 13:32:22 PST
Created attachment 451436 [details] Patch
Saam Barati
Comment 3 2022-02-09 14:56:50 PST
Comment on attachment 451436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451436&action=review I wonder if it's just better to change Weak to do the thing where it doesn't use m_impl in multiple places than to change the code to use opaque everywhere. I don't think llvm in practice will ever emit a load from a field twice when the program says to do it once, even if it can prove that such a heap location hasn't changed > Source/JavaScriptCore/runtime/WriteBarrier.h:140 > + explicit operator bool() const { return !!cell(); } > > - bool operator!() const { return !m_cell; } > + bool operator!() const { return !cell(); } Why do these need to use the opaque version?
Yusuke Suzuki
Comment 4 2022-02-09 15:00:29 PST
Comment on attachment 451436 [details] Patch Hmmm, not sure whether just using `m_impl` once is enough or not, probably it is not enough in terms of the C spec, but yeah, in the current build, it would be OK. I'll remove WTF::opaque for now.
Yusuke Suzuki
Comment 5 2022-02-09 15:03:05 PST
Created attachment 451450 [details] Patch
Yusuke Suzuki
Comment 6 2022-02-09 17:50:17 PST
Committed r289515 (247046@trunk): <https://commits.webkit.org/247046@trunk>
Radar WebKit Bug Importer
Comment 7 2022-02-09 17:51:16 PST
<rdar://problem/88725823>
Note You need to log in before you can comment on or make changes to this bug.