On Wed, 15 May 2024 18:45:16 GMT, ExE Boss <d...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java
>> line 104:
>>
>>> 102: // Optimistically try plain semantics first
>>> 103: final V v = value;
>>> 104: if (v != null) {
>>
>> If `value == null && state == NULL`, can the path still be constant folded?
>> I doubt it because `value` in this case may not be promoted to constant.
>
> Maybe the `state == NULL` check should be moved before `v != null`, as the
> **JIT** doesn’t constant‑fold `null` [`@Stable`] values:
> https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L41-L44
>
> https://github.com/openjdk/jdk/blob/8a4315f833f3700075d65fae6bc566011c837c07/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java#L64-L71
>
> [`@Stable`]:
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/vm/annotation/Stable.java
It seems reasonable to assume `null` values are not constant-folded. For
straight-out-of-the-box usage, there is no apparent significant difference as
indicated by a new benchmark I just added:
Benchmark Mode Cnt Score Error
Units
StableStaticBenchmark.atomic thrpt 10 5729.683 ? 502.023
ops/us
StableStaticBenchmark.dcl thrpt 10 6069.222 ? 951.784
ops/us
StableStaticBenchmark.dclHolder thrpt 10 5502.102 ? 1630.627
ops/us
StableStaticBenchmark.stable thrpt 10 12737.158 ? 1746.456
ops/us <- Non-null benchmark
StableStaticBenchmark.stableHolder thrpt 10 12053.978 ? 1421.527
ops/us
StableStaticBenchmark.stableList thrpt 10 12443.870 ? 2084.607
ops/us
StableStaticBenchmark.stableNull thrpt 10 13164.232 ? 591.284
ops/us <- Added null benchmark
StableStaticBenchmark.stableRecordHolder thrpt 10 13638.893 ? 1250.895
ops/us
StableStaticBenchmark.staticCHI thrpt 10 13639.220 ? 1190.922
ops/us
If the `null` value participates in a much larger constant-folding tree, there
might be a significant difference. I was afraid moving the order would have
detrimental effects on instance performance but that does not seem to be the
case:
Checking value first:
Benchmark Mode Cnt Score Error Units
StableBenchmark.atomic thrpt 10 246.460 ? 75.417 ops/us
StableBenchmark.dcl thrpt 10 243.481 ? 35.021 ops/us
StableBenchmark.stable thrpt 10 4977.693 ? 675.926 ops/us <-
Non-null
StableBenchmark.stableHoldingList thrpt 10 3614.460 ? 275.140 ops/us
StableBenchmark.stableList thrpt 10 3328.155 ? 898.202 ops/us
StableBenchmark.stableListStored thrpt 10 3842.174 ? 535.902 ops/us
StableBenchmark.stableNull thrpt 10 6217.737 ? 840.376 ops/us <-
null
StableBenchmark.supplier thrpt 10 9369.934 ? 1449.182 ops/us
Checking null first:
Benchmark Mode Cnt Score Error Units
StableBenchmark.atomic thrpt 10 275.952 ? 39.480 ops/us
StableBenchmark.dcl thrpt 10 252.697 ? 18.645 ops/us
StableBenchmark.stable thrpt 10 5211.552 ? 315.307 ops/us <-
Non-null
StableBenchmark.stableHoldingList thrpt 10 3764.202 ? 224.325 ops/us
StableBenchmark.stableList thrpt 10 3689.870 ? 419.858 ops/us
StableBenchmark.stableListStored thrpt 10 3676.182 ? 938.485 ops/us
StableBenchmark.stableNull thrpt 10 6046.935 ? 1512.391 ops/us <-
null
StableBenchmark.supplier thrpt 10 9202.202 ? 1479.950 ops/us
So, swapping order seems to be the right move.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1602719002
