On Thu, 1 May 2025 15:44:11 GMT, Vladimir Kozlov <k...@openjdk.org> wrote:
>>> assert(success || !AOTCodeCache::is_dumping_adapters(), ""); >> >> This condtion `!AOTCodeCache::is_dumping_adapters()` in the assert is not >> very intuitive. I think what we need to assert is future stores in the aot >> code cache are disabled. So having a method like >> `AOTCodeCache::is_store_disabled()` would better communicate the intent >> here. But I don't mind keeping this condition for this initial PR. I will >> just suggest to add a better assert message like: >> >> ```assert(success || !AOTCodeCache::is_dumping_adapters(), "storing of >> adapter must be disabled");``` >> >> And I think we should also be setting `_adapter_caching` to false in >> `report_load_failure` and `report_store_failure` to be consistent, otherwise >> we may end up in a situation where `AOTAdapterCaching` is false but >> `_adapter_caching` is true. In fact, I feel we should only be setting >> `_adapter_caching` and not `AOTAdapterCaching` in >> `report_load/store_failure` because `_adapter_caching` is the flag used to >> gate store/load operations. > > Thank you, @ashu-mehra. You have good points. I will work on them. FTR I suggested `!AOTCodeCache::is_dumping_adapters()` because that's the guarding check for `AOTCodeCache::store_code_blob()` call. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24740#discussion_r2070535559
