Re: Race condition in OnHeapHnswGraph

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

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/java=
/util/concurrent/atomic/Striped64.java

D.

On Fri, Jan 16, 2026 at 4:21=E2=80=AFPM Michael Sokolov <[email protected]=
> 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.durina=
@gmail.com>
> wrote:
> >
> > I'd like to do the PR, but could you please confirm to me that it's saf=
e
> 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@gmai=
l.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 <viliam.du=
[email protected]
> >
> > > wrote:
> > > >
> > > > Hi all,
> > > >
> > > > I'm looking at `OnHeapHnswGraph` code and noticed that the volatile
> 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 fact
> > > > single-threaded when documents are added to the index. It might be
> > > > 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]
>
>

--000000000000e7cffe06488313f9--