On Mon, 14 Apr 2025 15:46:54 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> This PR proposes to add the `@Stable` annotation to `j.l.String.hash` and
>> `j.l.String.hashIsZero`. This means the VM can trust these fields to never
>> change which enables constant folding optimizations.
>>
>> This PR is tested in tier1, tier2, tier3, and tier4 which all pass.
>
> Hello Per, I'm not too familiar with runtime compiler optimizations. So
> consider this as a basic question.
>
>> This means the VM can trust these fields to never change which enables
>> constant folding optimizations.
>
> If I'm not wrong, then it is the `hash` field value that we want to be
> considered as a constant (once computed) so that calls to `String.hashCode()`
> would get replaced with the constant computed value.
>
> Looking at the current implementation of `String.hashCode()`:
>
>
> public int hashCode() {
> // The hash or hashIsZero fields are subject to a benign data race,
> // making it crucial to ensure that any observable result of the
> // calculation in this method stays correct under any possible read of
> // these fields. Necessary restrictions to allow this to be correct
> // without explicit memory fences or similar concurrency primitives is
> // that we can ever only write to one of these two fields for a given
> // String instance, and that the computation is idempotent and derived
> // from immutable state
> int h = hash;
> if (h == 0 && !hashIsZero) {
> h = isLatin1() ? StringLatin1.hashCode(value)
> : StringUTF16.hashCode(value);
> if (h == 0) {
> hashIsZero = true;
> } else {
> hash = h;
> }
> }
> return h;
> }
>
>
> If I'm reading that correctly, and keeping aside concurrent calls from this
> discussion, then only one of `hash` or the `hashIsZero` fields will have its
> value changed to a non-default value. i.e. if `hashCode()` implementation
> computes a non-zero value then the `hash` field will be assigned a
> (non-default) value and if that method computes a hash of 0, then
> `hashIsZero` will get assigned a (non-default) value. It then means that the
> other field will never move out of its initial value and thus will never be
> considered "stable".
>
> Am I right? If yes, then would the runtime (hotspot) compiler still replace
> the call to `String.hashCode()` with a constant value?
Also re @jaikiran: yes, you are right that the current code cannot
constant-fold the scenario where the hash is 0; so `"".hashCode()` is not
constant as a result. The solution I shared above can address this scenario,
but it cannot completely bring performance to parity with other constant-folded
cases in Remi's shared benchmark (see
https://github.com/liachmodded/jdk/commit/247e8bd92e6dbad6df2dd50ad83caa49983a81b4)
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24625#issuecomment-2802495436
