RFR: 8344904: Interned strings in old classes are not stored in AOT cache

[David Holmes]

On Sat, 23 Nov 2024 03:25:44 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> Before this fix, CDS will only archive interned strings that are reachable from `ConstantPool::resolved_references()`, which is created only after a class is linked. However, old classes are not linked, so we didn't archive their interned strings.
> 
> It's possible for the Java mirror of an (unlinked) old class to point to interned strings. E.g.,
> 
> 
> public super class OldClassWithStaticString
>     version 49:0
> {
>     public static final Field s:"Ljava/lang/String;" = String "xxxx123yyyy456";
> 
> 
> The 's' field is initialized during class file parsing, so we must intern the `"xxxx123yyyy456"` string.
> 
> The fix is to collect interned strings from the static fields of unlinked classes.

Looks good, just a couple of nits.

Plus if this is only about the CDS archive can we say that in the title instead of AOT cache? IIUC this is an existing CDS issue and unrelated to any recent AOT/Leyden integrations - right?

Thanks

src/hotspot/share/oops/constantPool.cpp line 411:

> 409:   if (!ik->is_linked()) {
> 410:     // resolved_references() doesn't exist yet, so we have no resolved CONSTANT_String entries. However,
> 411:     // Some static final fields may have default values that were initialized when the class was parsed.

Suggestion:

    // some static final fields may have default values that were initialized when the class was parsed.

src/hotspot/share/oops/constantPool.cpp line 412:

> 410:     // resolved_references() doesn't exist yet, so we have no resolved CONSTANT_String entries. However,
> 411:     // Some static final fields may have default values that were initialized when the class was parsed.
> 412:     // We need to enter those into the CDS archived strings table.

Suggestion:

    // We need to enter those into the CDS archive strings table.

test/hotspot/jtreg/runtime/cds/appcds/sharedStrings/OldClassWithStaticString.jasm line 35:

> 33: 
> 34: public super class OldClassWithStaticString
> 35:     version 49:0

Is this a jasm file only because we need the version 49? If so please state that somewhere.

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22340#pullrequestreview-2456930916
PR Review Comment: https://git.openjdk.org/jdk/pull/22340#discussion_r1855654832
PR Review Comment: https://git.openjdk.org/jdk/pull/22340#discussion_r1855654994
PR Review Comment: https://git.openjdk.org/jdk/pull/22340#discussion_r1855656916