On Wed, 21 May 2025 00:28:35 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> It's not good to introduce a new symbol comparator in a random place. >> Symbol comparators should be defined and maintained centrally, in >> symbol.[ch]pp. >> >> If there's a bug with fast_compare, I want us to fix it eventually. So for >> now, define a companion function in symbol.hpp called fast_compare_cds_safe. >> We can work with it from there. You can use your proposed logic but we >> will probably want to fiddle with it, and probably merge it with >> fast_compare (per se) if CDS can be adjusted. > > This problem should happen only with dynamic CDS archives. > > With the static CDS archive (and AOT caches as well; `-XX:AOTCache` basically > are static CDS archives), Symbols are copied into the archive while > preserving the order of their relative addresses. > > However, when creating a dynamic CDS archive, we first load a static archive. > Let's say we load the static archive at address 0x800000000, a Symbol `A` in > the static archive could have an address of `0x800001000`. Now, we load a > class `K` who has a method named `B` but such a Symbol is not in the static > archive, this Symbol will be dynamically allocated from the Metaspace. Since > the address is decided by the OS, this symbol could have an address below the > static archive. E.g., `0x700000000`; > > Let's say `K` defines two methods, `A()` and `B()`, they are stored in the > `InstanceKlass::_methods` array for `K`. This array is sorted by ascending > addresses of the method's names. So `K::B()` comes before `K::A()`. > > When `K` is copied into the dynamic archive, the symbol `B` will also be > copied into the dynamic archive. The dynamic archive resides in a memory > region above the static archive, so `B` will have an address like > `0x800002000`. Therefore, we want the copy of `K` in the dynamic archive to > have a `_methods` array where `K::A()` comes before `K::B()`. > > This is done in > [`DynamicArchiveBuilder::sort_methods()`](https://github.com/openjdk/jdk/blob/cedd1a5343dceb5394b8ed5ea78bb717f05c8caf/src/hotspot/share/cds/dynamicArchive.cpp#L138) > > My recommendation is to sort your table using `fast_compare()`, and re-sort > this table in the same fashion as `DynamicArchiveBuilder::sort_methods()`.
Thanks for the pointers; in fact I had the problem with a (intermediate) variant that encoded the symbol addresses right into the stream. Therefore the issue was comparing value of symbol values before CDS persisting those and after dehydration. Since we're comparing only symbols within single class, I hope that the issue you describe above won't happen. I'll keep the problem with dynamic archives in mind, though. With the current code I can try using `fast_compare` again and get back if I find any problems again. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24847#discussion_r2099806937
