Hi Stephan, On Sun, 28 May 2023 at 14:00, Stephan Markwalder <step...@markwalder.net> wrote: > > Experimentally using a simple non-volatile class field for the cache > works better: the fact that multiple threads see a different cached value > is actually an advantage here. Also apparently the cost of syncing the > cached value among threads is not compensated by a smaller number of cache > misses. > > When I look at code where thread-safety has been given up for performance, > I'm always very much scared of potential side-effects like partially > initialized Java objects made visible to other threads. In this specific > case, the fields of `CachedTime` are non-final and non-volatile. Wouldn't > this mean that another thread could read their uninitialized valued? Due to > the method's logic, this would probably not be a big deal for > `epochSeconds` and `nanoOfSecond` (just a cache miss), but an unexpected > null value read from the field `formatted` could cause a new bug. I assume > this could be fixed with an extra null-check, or by making all fields in > `CachedTime` final. But it is a tricky territory nevertheless, and every > future code change has to be reviewed very carefully in this regard.
Thanks for the review, I'll finalize the fields of the `CachedTime` class. If you have additional remarks feel free to review the PR directly: reviews are not restricted to the core developers. Piotr
