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

[jianglizhou]

Please review the fix that uses String type for the mapped value in ModuleLoaderMap.Mapper map (Map<String, String>). Please see details in https://bugs.openjdk.org/browse/JDK-8342642, thanks.

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21672/head:pull/21672
$ git checkout pull/21672

Update a local copy of the PR:
$ git checkout pull/21672
$ git pull https://git.openjdk.org/jdk.git pull/21672/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21672

View PR using the GUI difftool:
$ git pr show -t 21672

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21672.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back jiangli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

[openjdk[bot]]

@jianglizhou The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge[bot]]

Webrevs

• 01: Full - Incremental (94f35861)
• 00: Full (ade90fb5)

[AlanBateman]

@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]

@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.

[jianglizhou]

@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%3Acomment-tabpanel#comment-14714939.

[jianglizhou]

I brought up this issue in today's Leyden/premain meeting. There was a good discussion on the specific issue, and the general Integer cache archiving and identity hash problem. The conclusion was it's better to avoid using boxed Integer and go with String in ModuleLoaderMap.Mapper.

[jianglizhou]

@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%3Acomment-tabpanel#comment-14714939.

I just re-looked at the IntegerCache 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.

[shipilev]

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.

Phew, thanks! I thought I was misunderstanding some fundamental thing here :) I think IntegerCache interaction with CDS archive deserves a fix regardless. Have you filed the bug for it, @jianglizhou? AFAICS, if we fix IntegerCache <-> CDS interaction, we solve this particular problem as well.

I am still non-committal about this special fix. We can still do it, but then this patch effectively changes relying on boxing identity behavior over Integers to relying on interning behavior over Strings, right? If we want to be 100% safe, shouldn't == checks be rewritten to equals? And when we do so, would that affect startup in any meaningful way?

The names of the fields should probably be changed from _INDEX to something else, like _NAME?

[AlanBateman]

The conclusion was it's better to avoid using boxed Integer and go with String in ModuleLoaderMap.Mapper.

Okay, you'll need to rename the fields and Mapper's description will need to be updated as explains why a boxed value was used.

[jianglizhou]

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.

Phew, thanks! I thought I was misunderstanding some fundamental thing here :) I think IntegerCache interaction with CDS archive deserves a fix regardless. Have you filed the bug for it, @jianglizhou? AFAICS, if we fix IntegerCache <-> CDS interaction, we solve this particular problem as well.

I filed https://bugs.openjdk.org/browse/JDK-8343019. I agree with what John said during the meeting yesterday, let's move away from cached Integer in this case (ModuleLoaderMap).

I am still non-committal about this special fix. We can still do it, but then this patch effectively changes relying on boxing identity behavior over Integers to relying on interning behavior over Strings, right? If we want to be 100% safe, shouldn't == checks be rewritten to equals? And when we do so, would that affect startup in any meaningful way?

Right, it's based on archived interned String behavior. We don't disable any of the archived interned Strings at runtime. So I think we are indeed safe.

In an offline chat with @cushon yesterday, Liam also asked about changing ==. My sense was that there were probably performance considerations when the change was originally made and == was chosen (instead of .equals()) for performance reason. I think the performance difference mostly would be within noise level. Seems okay to change .equals(). I can do some measurements to compare.

The names of the fields should probably be changed from _INDEX to something else, like _NAME?

Ok.

[jianglizhou]

There is an issue from my last push. It includes unintended merged changes from head. It's probably because I did a cherrypick for my fix when creating the initial PR. Let me close this PR and create a new one. I'll link all existing comment threads from this PR to the new PR.

[jianglizhou]

There is an issue from my last push. It includes unintended merged changes from head. It's probably because I did a cherrypick for my fix when creating the initial PR. Let me close this PR and create a new one. I'll link all existing comment threads from this PR to the new PR.

I moved the change to a clean branch and created https://github.com/openjdk/jdk/pull/21722.