On Thu, 23 Apr 2026 12:06:30 GMT, Marc Chevalier <[email protected]> wrote:
>> `acmp` for value object might be expensive: it may need substitutability >> test, which is done through a Java call. That is the last resort when >> everything failed, that is: we are in the presence of two value objects of >> the same class, not statically known or speculated (profile absent or >> polluted). While a lot of the acmp logic is trying to improve acmp at >> compile-time, this change proposes a runtime check to take a fast path. >> >> The idea is that if the object is small enough, we can read all the fields >> at once with a single 8-byte load, we exclude the non-interesting bits with >> a mask, and we can simply compare the masked value from both objects. >> >> The first step happens at class loading. When the class is loaded, we check >> whether all the fields can be read at once, from which offset, and the mask >> to filter out useless bits. Useless bits may have two sources: >> - padding between fields >> - header: this happens because we cannot necessarily start reading at >> `payload_offset`, the payload might be smaller than a long, and we could >> overread. On the other hand, the header is always long enough (never less >> than 8 bytes), so we may read from somewhere in the header to make sure we >> don't go further than the payload. This piece of header must be filtered out. >> >> This strategy may not apply if the object is too big, or if it contains an >> oop. Loading oop without precaution, concurrently with the GC is not a good >> idea. This limitation is rather harmless: this fast path is mostly made not >> to pay the price of a complicated `acmp` for magically migrated classes >> (such as `Integer`), which are small and don't contain oops. >> >> There is an interesting corner case: the mask can be 0 for an empty value >> class. We use a negative offset to signal that the fast path doesn't apply, >> since it is not a valid value for the offset: we shouldn't load before the >> object. >> >> The second step is to prepare the runtime check. In `do_acmp`, when we used >> to prepare the Java call, now we have a parallel fast path. The condition >> uses the fact that we already have the class node available (used for >> checking operands have the same types). We can get the fast acmp offset from >> the class, check if it looks valid (that is non-negative), and if so, take >> the fast path that will load the mask, do the two loads and masking, and >> compare the given values. If the class of the operand is known precisely at >> parsing, we don't emit the fast path since the call will be nicely >> intrinsified. >> >> The thir... > > Marc Chevalier has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetics I have a couple of comments about this code. src/hotspot/share/classfile/classFileParser.cpp line 5430: > 5428: > 5429: #else > 5430: /* Leaving the mask and offset to default values (that is just > return;") is a correct and easy way to implement this for other endianness, > but it will Drive by comments: The hotspot coding standard is to use // comments not /* */ ones. Can you change these and all the ones below? src/hotspot/share/classfile/classFileParser.cpp line 5436: > 5434: Unimplemented() > 5435: #endif > 5436: } I think this and the method above this aren't reallly parsing any classfiles. They should be moved to be members of InstanceKlass instead. src/hotspot/share/classfile/classFileParser.cpp line 5437: > 5435: #endif > 5436: } > 5437: This is very complicated and needs more commentary in the source file about what all this compression and casting is about. If it only yields a small performance increase, should we really have this? ------------- PR Review: https://git.openjdk.org/valhalla/pull/2353#pullrequestreview-4164493424 PR Review Comment: https://git.openjdk.org/valhalla/pull/2353#discussion_r3132585469 PR Review Comment: https://git.openjdk.org/valhalla/pull/2353#discussion_r3132626065 PR Review Comment: https://git.openjdk.org/valhalla/pull/2353#discussion_r3132648432
