Re: Race condition in OnHeapHnswGraph

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@g=
mail.com> 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@gmail.=
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.duri=
[email protected]>
> > wrote:
> > >
> > > Hi all,
> > >
> > > I'm looking at `OnHeapHnswGraph` code and noticed that the volatile f=
ield
> > > `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, an=
d
> > 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]
> >
> >