On Mon, 20 Oct 2025 21:36:54 GMT, Stefan Karlsson <[email protected]> wrote:
>> Okay. @stefank is having a look at fixing this. > > I've sort-of implemented the second part of your comment about removing the > tri-state: > https://github.com/fisk/jdk/compare/8326035_JEP_object_streaming_v6...stefank:jdk:8326035_JEP_object_streaming_v6_stefank > > The enum had *four* states (`uninitialized`, `mapping`, `streaming`, `none`) > and while reading this PR the `none` part seemed unusual. After working on > the proposed patch I think the `none` value served a purpose, but let's see > if a patch without it makes sense. > > The patch still uses an enum instead of two booleans. I think that makes the > code more readable because we encode the three states with three enum values. > If we were to use two booleans we would end up with four possible states and > then the reader of the code would have to figure out which of the forth state > is an invalid state. I think what we have in the patch above is safer. (Let's > revisit this if this still is a blocker) > > So, I removed `none` and added strict asserts that you are not allowed to ask > for the mode until the variable has transitioned away from `uninitialized` to > either `mapping` or `streaming`. After the variable has been initialized the > questions are binary and you have `is_loading_mapping_mode() == > !is_loading_streaming_mode()`. I don't see the appeal to remove one of these > two functions, because that just adds negation to the code and that makes it > harder to read. (We can experiment with that in a separate patch) > > I tried to take it even further and join these `is_loading` functions with > the `is_archived_heap_in_use` and the `is_in_use` properties, but that hit > problems with error-edge cases. I have a feeling that the `none` value dealt > with these problems. > > Some more details about the edge-case problems: > > During loading of the heap archive there is a duration where we have decided > that we are going to load the archived heap and what loading mode to use, but > the archive has not been loaded enough have the `is_archived_heap_in_use` > function return `true`. During these early stages we must use the > `is_loading` and `is_loading_` functions. After `is_archived_heap_in_use` > starts to return the correct value, the code likely needs to use that > function instead of the `is_loading` functions. Some parts of the AOT/CDS > code will bug out if you try to use the `is_loading` functions after an error. > > So, I've tried to keep the usages of `is_loading` functions inside AOT/CDS > code. The `is_archived_heap_in_use`, or one of it's sub-components (E.g. > `AOTMappedHeapLoader:is_in_use()`), are... Thank you for the contribution, @stefank. I added it to the PR. @iklam hope this addresses your concerns. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27732#discussion_r2448732105
