RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper

[Jiangli Zhou]

On Thu, 24 Oct 2024 05:51:38 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> @iklam Would it be possible to comment on this?  If the system properties to configure the range of integer cache are set at runtime, does it just not use this archived object graph? I'm wondering if it should disable CDS?
>
>> @iklam Would it be possible to comment on this? If the system properties to configure the range of integer cache are set at runtime, does it just not use this archived object graph? I'm wondering if it should disable CDS?
> 
> A better fix would be:
> 
> If runtime java.lang.Integer.IntegerCache.high < dumptime java.lang.Integer.IntegerCache.high
>   -- copy the initial elements from the dumptime cache array
>   -- fill the rest of the cache array
> 
> that way, we will still preserve the object identity of the Integers created during dump time.
> 
> I think we need to do the same thing for the other box cases.

> > > @iklam Would it be possible to comment on this? If the system properties to configure the range of integer cache are set at runtime, does it just not use this archived object graph? I'm wondering if it should disable CDS?
> > 
> > 
> > A better fix would be:
> > If runtime java.lang.Integer.IntegerCache.high < dumptime java.lang.Integer.IntegerCache.high -- copy the initial elements from the dumptime cache array -- fill the rest of the cache array
> > that way, we will still preserve the object identity of the Integers created during dump time.
> > I think we need to do the same thing for the other box cases.
> 
> I've been thinking about something like that as a longer term fix after this quick fix. However, there are some complications, e.g.:
> 
> When an archived boxed Integer instance is 'used' in a pre-initialized class static field's sub-graph, depending on the runtime IntegerCache.high specified by the system property, the specific 'used' Integer may not be within the runtime range (used_Integer > IntegerCache.high). In that case, the runtime modified cache does not contain the archived boxed Integer instance. Then, the usage of the archived instance is invalid, which causes the corresponding pre-initialized static field to still be problematic. For this loader map, in an extreme example (unlikely to happen) when runtime IntegerCache.high=1, it would still not work well. For such cases, we would need to invalidate the pre-initialized static field or class. That's the reason I mentioned filing a separate bug for the archived Integer cache and evaluating together with the general AOT cache work, see https://bugs.openjdk.org/browse/JDK-8342642?focusedId=14714939&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acommen
 t-tabpanel#comment-14714939.

I just re-looked at the [IntegerCache](https://github.com/openjdk/jdk/blob/d1540e2a49c7a41eb771fc9896c367187d070dec/src/java.base/share/classes/java/lang/Integer.java#L961) code, I need to take the above back. Unlike I remembered, IntegerCache doesn't recreate the cache if runtime cache length () is **less** than the archived cache:


   if (archivedCache == null || size > archivedCache.length) {
      // re-create
  }


In that sense, I think we don't have a risk of potentially with `used_Integer > IntegerCache.high`. The idea described in Ioi's comment above (also brought up by Aleksey Shipilev separately during premain meeting) could be sufficient.

Anyway moving away from archived boxed Integer and identity hash comparison in this case is safer.

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

PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2436358591