On Mon, 26 May 2025 17:08:45 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
> SonarCloud complains that since > [JDK-8356080](https://bugs.openjdk.org/browse/JDK-8356080) we are using > `Boolean` boxes in `ReverseOrderListView`. This change `boolean` -> `Boolean` > was made in [JDK-8356080](https://bugs.openjdk.org/browse/JDK-8356080) to > allow `@Stable` folding of boolean field. But it is very awkward to trade in > the existence of the boxed object to allow optional constant folding. > > We can flatten this field to `byte` and check the specific non-zero values. > The field is final, so it is never actually in `0` state. > > Additional testing: > - [x] Linux x86_64 server fastdebug, `java/util` A bunch of things are going on here. First, yes, changing a `boolean` to `Boolean` in order to gain constant folding seems dubious, against a potential cost of a dependent load. I note further that the `Boolean` class itself has a private final boolean `value` field that itself isn't marked `@Stable` so do we really get constant folding here? Maybe we should look at the `Boolean` class instead? The conversion to a positive/negative byte here is clever. It's probably a useful technique in some contexts. (Maybe it's worth applying inside the `Boolean` class itself.) Here, however, it feels to me like the whole exercise is just fiddling around hoping to optimize something in the absence of an actual performance problem. Let's step back a bit and look at the context. The reverse-ordered view is mostly intended as a temporary view into a backing list, for searching or copying in reverse. I doubt that anyone will do any highly performance-sensitive operations on such a list. If there is unnecessary overhead, it should be eliminated of course, but I don't really see that going on here. The `modifiable` flag is used only for error checking in mutator methods. It's not checked at all for read-only methods. If the field's value is true, since it's `@Stable final`, this may allow for some constant folding if a mutation is done within an inner loop. As noted originally, the false stable value won't be constant folded. However if the value is false, the mutator methods always throw an exception. This occurs when somebody attempts a mutation operation on an unmodifiable list. From the collection API's point of view this is treated as a programming error. I claim that we don't really care much about the performance of such cases. I think we should just revert this back to a primitive `boolean` field. I could go either way on it being `@Stable`. I don't think it helps much but maybe it doesn't hurt either. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25456#issuecomment-2913291278
