On Mon, 9 Sep 2024 12:21:19 GMT, Thomas Schatzl <tscha...@openjdk.org> wrote:
>> Roman Kennke has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Try to avoid lea in loadNklass (aarch64) >> - Fix release build error > > src/hotspot/share/oops/oop.hpp line 134: > >> 132: inline Klass* forward_safe_klass(markWord m) const; >> 133: inline size_t forward_safe_size(); >> 134: inline void forward_safe_init_mark(); > > Given the comment these methods do not seem "safe" to me. Maybe use "raw" or > something to better indicate that care must be taken to use them. > > Maybe the "safe" refers to use them only in "safe" contexts, but in Hotspot > code iirc we use something like "raw" or "unsafe". Restating my earlier comment about this: These functions are mainly used by the GCs. In one of the patches I've cleaned away all usages except for those in Shenandoah. I would prefer to see these completely removed from the oops/ directory and let the GCs decide when and how to perform "safe" reads of these values. > src/hotspot/share/oops/oop.hpp line 356: > >> 354: return mark_offset_in_bytes() + sizeof(markWord) / 2; >> 355: } else >> 356: #endif > > Maybe instead of trying to calculate some random, meaningless value just use > some "random" value directly? > I am fine with the existing code, but first stating directly that "any value" > works here, this additional code seems to confuse the message. (Fwiw, the > method is also used during Universe initialization). Just to be clear, the second part of the quoted sentence is important: > could be any value *that is not a valid field offset* ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750428581 PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750432186
