Bug 236387 - Use local variable pointer for concurrently load value
Summary: Use local variable pointer for concurrently load value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-09 11:28 PST by Yusuke Suzuki
Modified: 2022-02-09 17:51 PST (History)
7 users (show)

See Also:


Attachments
Patch (15.05 KB, patch)
2022-02-09 11:33 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.16 KB, patch)
2022-02-09 13:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (14.86 KB, patch)
2022-02-09 15:03 PST, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2022-02-09 11:28:35 PST
Use WTF::opaque in some places to ensure access is done only once
Comment 1 Yusuke Suzuki 2022-02-09 11:33:01 PST
Created attachment 451418 [details]
Patch
Comment 2 Yusuke Suzuki 2022-02-09 13:32:22 PST
Created attachment 451436 [details]
Patch
Comment 3 Saam Barati 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?
Comment 4 Yusuke Suzuki 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.
Comment 5 Yusuke Suzuki 2022-02-09 15:03:05 PST
Created attachment 451450 [details]
Patch
Comment 6 Yusuke Suzuki 2022-02-09 17:50:17 PST
Committed r289515 (247046@trunk): <https://commits.webkit.org/247046@trunk>
Comment 7 Radar WebKit Bug Importer 2022-02-09 17:51:16 PST
<rdar://problem/88725823>