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

[Jiangli Zhou]

On Fri, 25 Oct 2024 10:45:00 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

> > 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 `Integer`s 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.

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

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