On Mon, 28 Oct 2024 10:16:40 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:

> This is forked from 
> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) and filed as a 
> general issue for archived boxed Integer cache when it's recreated at 
> runtime. In short, current code drops the entire primitive cache when the CDS 
> archived version of the cache is too short. This poses a problem with code 
> that uses CDS archived cache instances, since the boxed equality would break 
> when comparing the CDS-archived value and the IntegerCache value recreated at 
> runtime.
> 
> Ioi suggested a fix here: 
> https://github.com/openjdk/jdk/pull/21672#issuecomment-2434359711. I 
> separately arrived to the same idea. This PR implements it. `IntegerCache` 
> gets the special treatment, because it is the only cache that can be tuned. 
> Other caches just prevent the use of bad archived cache (which I think should 
> realistically never happen) without re-creating it.
> 
> Unfortunately, I was unable to create a standalone CDS test for this: I 
> somehow need to be able to put my own archived `Integer` instance into the 
> archive, and I don't clearly see how. I tested this patch with original 
> reproducer from [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) -- 
> and it starts to pass. I don't think we would be able to convert that 
> reproducer into a unit test, since 
> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) itself would make 
> the reproducer ineffective.
> 
> Additional testing:
>  - [x] macos-aarch64-server-fastdebug, 
> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) reproducer now 
> passes
>  - [x] linux-aarch64-server-fastdebug, `all`

I have mixed feelings about proposed fix. It does look like an improvement over 
current behavior, but does it really address the root cause of the problem?

As an example, if `IntegerCache.high` is set to `1` during archiving, then 
`ModuleLoaderMap.PLATFORM_LOADER_INDEX=1` ends up pointing to archived instance 
and `ModuleLoaderMap.APP_LOADER_INDEX=2` points to non-archived instance which 
are then persisted in the archive as part of the sub-graph rooted at 
`ModuleLoaderMap.map`.

If larger `IntegerCache.high` is set during subsequent runs, both 
`ModuleLoaderMap.PLATFORM_LOADER_INDEX` and `ModuleLoaderMap.APP_LOADER_INDEX` 
are both populated with shared instances which breaks pointer comparison in 
`ModuleLoaderMap.apply()`.

Do I miss something important here? I briefly looked through previous 
discussions, but didn't see this particular case covered.

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

PR Comment: https://git.openjdk.org/jdk/pull/21737#issuecomment-2442848703

Reply via email to