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 AM Viliam Ďurina <[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 AM Michael Sokolov <[email protected]> wrote: > > > yeah this looks silly: do you want to open a PR to fix? > > > > On Mon, Jan 12, 2026 at 6:39 AM Viliam Ďurina <[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 = graphRamBytesUsed; > > > graphRamBytesUsed = bytesUsed + l; > > > > > > This is equivalent to `graphRamBytesUsed += 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]
