On Wed, 30 Oct 2024 19:11:46 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> A thought for a possible cleanup, after this PR is done… >> >> The scratch mirror logic had me… scratching my head. It seems to me that a >> more descriptive name would make the code explain itself better. I suggest >> (for a future cleanup) calling a mirror structure which is being >> aot-assembled (just for the archive) a "future mirror" (or maybe "production >> mirror" or "mirror asset"). This is in distinction to the current "live" >> mirror, which is also the AOT phase mirror. In general, from the point of >> view of the assembly phase, when we build new structures not created by the >> JVM as a result of post-training Java execution, we might want to give them >> a common descriptive term. But I suppose most items that make it into the >> AOT cache are faithful copies of live data, present in the VM at dump time >> (end of assembly phase). In that case, it's more like "the same structure" >> in all cases, and there's no need to simultaneously work on both present and >> future versions of the same structure. >> >> (When I say "structure" I mean mirror object for now, but perhaps the >> pattern might expand to something else? Or, maybe we will get rid of the >> two-mirror solution, in which case every future structure is also completely >> present and live in the assembly-phase VM.) > >> A thought for a possible cleanup, after this PR is done… >> >> The scratch mirror logic had me… scratching my head. It seems to me that a >> more descriptive name would make the code explain itself better. I suggest >> (for a future cleanup) calling a mirror structure which is being >> aot-assembled (just for the archive) a "future mirror" (or maybe "production >> mirror" or "mirror asset"). This is in distinction to the current "live" >> mirror, which is also the AOT phase mirror. In general, from the point of >> view of the assembly phase, when we build new structures not created by the >> JVM as a result of post-training Java execution, we might want to give them >> a common descriptive term. But I suppose most items that make it into the >> AOT cache are faithful copies of live data, present in the VM at dump time >> (end of assembly phase). In that case, it's more like "the same structure" >> in all cases, and there's no need to simultaneously work on both present and >> future versions of the same structure. >> >> (When I say "structure" I mean mirror object for now, but perhaps the >> pattern might expand to something else? Or, maybe we will get rid of the >> two-mirror solution, in which case every future structure is also completely >> present and live in the assembly-phase VM.) > > The cached heap objects are mostly copied as-is, with a recursive walk from a > set of roots. However, in some cases, we need to perform transformation in > some of the objects. The transformation is implemented by substituting some > of the discovered objects with a "scratch" (or "future") version. > > For example, for java mirrors: > > - If a class K1 *is not* aot-initialized, we need to zero out most of the > fields inside `K1->java_mirror()`, but keep the injected `klass` and > `array_klass` native pointers. > - If a class K2 *is* aot-initialized, we need to also keep the static fields > declared in Java code in `K2->java_mirror()` > > For example, here are the contents of the aot-cached mirror for the > java/lang/String class: > > > - ---- fields (total size 17 words): > - private volatile transient 'classRedefinedCount' 'I' @12 0 (0x00000000) > - injected 'klass' 'J' @16 2684621920 (0x00000000a0041460) > - injected 'array_klass' 'J' @24 2684707368 (0x00000000a0056228) > - injected 'oop_size' 'I' @32 17 (0x00000011) > - injected 'static_oop_field_count' 'I' @36 2 (0x00000002) > - private volatile transient 'cachedConstructor' > 'Ljava/lang/reflect/Constructor;' @40 null (0x00000000) > - private transient 'name' 'Ljava/lang/String;' @44 null...
> @iklam I remember there was > [ConstantPool::iterate_archivable_resolved_references](https://github.com/iklam/jdk/blob/f0bc1ae60fea80d5914d520457986a753e75fae4/src/hotspot/share/oops/constantPool.cpp#L288) > in #21143 but I don't see it any more in this PR. Can you please comment on > why was that removed? An indy call site has two entries in the resolved_references array: - (a) The resolved call site - (b) The MethodHandle of the boot strap method We weren't archiving (b), which caused some JIT replay code to fail (the JIT assumes that both (a) and (b) must be resolved together). To fix this problem, I added the archiving of (b) in https://github.com/openjdk/jdk/pull/21642/commits/1d3daa4299691e75450d56ed1608b77e75cfd00a Afterwards, I realized that we can simply archive everything in the resolved_reference array, because AOTConstantPoolResolver::is_indy_resolution_deterministic() already ensures that we only resolve indys that we can support. `ConstantPool::iterate_archivable_resolved_references()` is no longer necessary as it simply repeats the checks in `is_indy_resolution_deterministic()`. That's why I removed it in https://github.com/openjdk/jdk/pull/21642/commits/1d3daa4299691e75450d56ed1608b77e75cfd00a But, now I realized that this part is also removed, which is probably not good. Array<ResolvedMethodEntry>* method_entries = cache()->resolved_method_entries(); if (method_entries != nullptr) { for (int i = 0; i < method_entries->length(); i++) { ResolvedMethodEntry* rme = method_entries->adr_at(i); if (rme->is_resolved(Bytecodes::_invokehandle) && rme->has_appendix() && cache()->can_archive_resolved_method(this, rme)) { int rr_index = rme->resolved_references_index(); assert(resolved_reference_at(rr_index) != nullptr, "must exist"); function(rr_index); } } } For safety, I will revert the removal. We can consider refactoring the code after 483 is integrated. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21642#issuecomment-2448302959
