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 AM 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/java/util/concurrent/atomic/Striped64.java > > D. > > On Fri, Jan 16, 2026 at 4:21 PM 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 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] > > > > >
