On Thu, 16 May 2024 06:54:26 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> 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 am afraid moving the order would > have detrimental effects on instance performance: > > 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 287.640 ? 17.858 ops/us > StableBenchmark.dcl thrpt 10 250.398 ? 20.874 ops/us > StableBenchmark.stable thrpt 10 3745.885 ? 1040.534 ops/us <- > Non-null > StableBenchmark.stableHoldingList thrpt 10 2982.129 ? 503.492 ops/us > StableBenchmark.stableList thrpt 10 3125.045 ? 416.792 ops/us > StableBenchmark.sta... I think the result would be more convincing if the stable case is changed to `sum += (stable.orThrow() == null ? 0 : 1) + (stable2.orThrow() == null ? 0 : 1);` as adding by 1 might be somewhat better optimized by JIT. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1603148915
