RFR: 8354300: Fields in String are not trusted

[Chen Liang]

On Mon, 14 Apr 2025 15:46:54 GMT, Jaikiran Pai <jpai at 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