On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
I don't think saving one volatile read is a reasonable trade-off for the loss
of readability of this code change.
-
PR
On Fri, 30 Jun 2023 08:30:45 GMT, Raffaello Giulietti
wrote:
> In the end, this PR is not about fixing a race, as the title seems to suggest
> (the original code is correct), but to avoid a volatile read, right?
Yeah, probably I was wrong in my conclusion. Should I rename the ticket?
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
True. I was mislead because the return statement does not appear in the
changeset (the line is textually the same, although it has a
On Thu, 29 Jun 2023 12:23:11 GMT, Raffaello Giulietti
wrote:
>> Double-checked locking should rely on local variable to avoid racy reads
>> from volatile field.
>
> The role of the local `keySet` seems pretty useless. It doesn't save neither
> volatile reads nor writes.
@rgiulietti `keySet`
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
Retrieving the approval, per the discussion.
-
Changes requested by naoto (Reviewer).
PR Review:
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
Yeah, current code looks like a correct volatile-bearing-DCL. `keySet` is
safely published already. Introducing local variable gains us
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
Given all the comments, I don't think this is ready to integrate.
-
Changes requested by rriggs (Reviewer).
PR Review:
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
The new code makes it more visible that 1. keySet is expected to be volatile
and 2. that there is no path were code could be reordered
On Thu, 29 Jun 2023 10:08:27 GMT, Sergey Tsypanov wrote:
>> Looks ok; was this a tool detected race?
>
> @RogerRiggs no tool, just fell into the sources while debugging my application
@stsypanov I don't see that there's any issue with the existing code as the
volatile field is only written at
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
The role of the local `keySet` seems pretty useless. It doesn't save neither
volatile reads nor writes.
-
PR Comment:
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
I don't see any race here. The addition of the local serves no purpose AFAICS -
we don't even save any volatile reads.
-
PR
On Wed, 28 Jun 2023 20:02:58 GMT, Roger Riggs wrote:
>> Double-checked locking should rely on local variable to avoid racy reads
>> from volatile field.
>
> Looks ok; was this a tool detected race?
@RogerRiggs no tool, just fell into the sources while debugging my application
-
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
While I still cannot see any malignancy in that race, the proposed change makes
the code simpler to reason about and probably faster to
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
+1
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14692#pullrequestreview-1503927831
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
Looks ok; was this a tool detected race?
-
Marked as reviewed by rriggs (Reviewer).
PR Review:
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
It'd be a benign race in case all members of `HashSet` are `final`, but they
aren't, so there are no safe publication guarantees.
On Wed, 28 Jun 2023 11:05:11 GMT, Sergey Tsypanov wrote:
> Double-checked locking should rely on local variable to avoid racy reads from
> volatile field.
To put this into perspective, the race you are trying to fix seems benign, so
the PR qualifies as a performance improvement, not a bug
17 matches
Mail list logo
