Re: Race condition in OnHeapHnswGraph

--000000000000e6f32d064885d8ee
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Wow this is really cool -- thanks for sharing Dawid.

What an incredibly tricky concurrent "sum" operator!  I love the dynamic
detection of contention (CAS failures), only creating enough
"thread-privateness" as needed given the current situation (how many
threads are actually trying to sum concurrently).

Mike McCandless

http://blog.mikemccandless.com


On Fri, Jan 16, 2026 at 10:35=E2=80=AFAM Dawid Weiss <[email protected]=
> wrote:

> Orthogonal, but isn't it quite mind-boggling how a conceptually simple
> "sum" becomes a hairy beast when the implementation
> tries to be efficient. If you haven't seen this class and would like to
> read Doug Lea's comments, I highly recommend them.
>
>
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/ja=
va/util/concurrent/atomic/Striped64.java
>
> D.
>
> On Fri, Jan 16, 2026 at 4:21=E2=80=AFPM Michael Sokolov <[email protected]=
om>
> wrote:
>
> > Sorry I thought you were simply proposing to eliminate the unneeded
> > local variable.
> >
> > We do have the possibility of concurrent indexing under which multiple
> > threads can be adding nodes to the same graph data structure, and
> > increasing its size, so this does need to be thread-safe.  I'm
> > guessing the volatile variable was a low-cost attempt to propagate the
> > information as a "best effort". At the end of the day this total size
> > is not something we rely on for correctness, so if it misses some
> > bytes here and there we can tolerate it. Maybe Ben Trent can share his
> > thoughts. It isn't broken in any way that we would probably notice,
> > but using a LongAdder certainly is more.
> >
> > On Fri, Jan 16, 2026 at 3:44=E2=80=AFAM Viliam =C4=8Eurina <viliam.duri=
[email protected]>
> > wrote:
> > >
> > > I'd like to do the PR, but could you please confirm to me that it's
> safe
> > to
> > > use non-volatile field? I believe this should be the case.
> > >
> > > Viliam
> > >
> > > On Thu, Jan 15, 2026 at 12:25=E2=80=AFAM Michael Sokolov <msokolov@gm=
ail.com>
> > wrote:
> > >
> > > > yeah this looks silly: do you want to open a PR to fix?
> > > >
> > > > On Mon, Jan 12, 2026 at 6:39=E2=80=AFAM Viliam =C4=8Eurina <
> [email protected]
> > >
> > > > wrote:
> > > > >
> > > > > Hi all,
> > > > >
> > > > > I'm looking at `OnHeapHnswGraph` code and noticed that the volati=
le
> > field
> > > > > `graphRamBytesUsed` is modified in `addNode` in a racy way:
> > > > >
> > > > >   long bytesUsed =3D graphRamBytesUsed;
> > > > >   graphRamBytesUsed =3D bytesUsed + l;
> > > > >
> > > > > This is equivalent to `graphRamBytesUsed +=3D l`.
> > > > >
> > > > > This code is susceptible to lost update due to non-atomic
> > > > read-modify-write
> > > > > operation.
> > > > >
> > > > > I guess the it's not really a problem, because this code is in fa=
ct
> > > > > single-threaded when documents are added to the index. It might b=
e
> > > > > concurrent during merging, but then `ramBytesUsed()` isn't called=
,
> > and
> > > > it's
> > > > > a wasted work.
> > > > >
> > > > > If the above assumption is correct, then this field should not be
> > > > volatile
> > > > > to improve performance. If it's not, it should be replaced with
> > > > > `AtomicLong` or `LongAdder`.
> > > > >
> > > > > Viliam
> > > >
> > > > -------------------------------------------------------------------=
--
> > > > To unsubscribe, e-mail: [email protected]
> > > > For additional commands, e-mail: [email protected]
> > > >
> > > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [email protected]
> > For additional commands, e-mail: [email protected]
> >
> >
>

--000000000000e6f32d064885d8ee--