On Tue, 10 Nov 2020 11:01:54 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> I have a question and a couple of minor suggestions on new test. > Q: Why the value of ITERS is that big? What is the need to have this number > of iterations? The test verifies the answer does not change if we hit JIT compilers in that code. Since we are doing C1/C2 intrinsics, we need to execute the loops more than compilation-threshold times. Since the current threshold is about 100K, doing 1M iterations seems good: it is well past the compilation threshold times, and there is time to re-enter the newly compiled method. The test run time is still sane, 1 minute on my Linux x86_64 fastdebug. I can do 200K iterations and -Xbatch instead, though, see new change. This drops the test run time to 30 seconds. > Also, I do not like one-letter identifiers, especially if they are not local. > Could you, please, replace identifiers R and A with some short versions that > give a hint. > Something like REFSIZE and ALIGNMENT would be good enough. Renamed to `REF_SIZE` and `OBJ_ALIGN` instead. > Also, what tests did you run to make sure no regression is introduced? Old code calls into `oop::size()` to get the object size. That method decodes the object's layout helper. So when we replace it with intrinsic, we now have to test the different shapes of the layout helper and varying conditions for that decoding. So the new test tries to cover the comprehensive matrix: - the usual object shapes: objects, primitive arrays, object arrays; - different compressed oops modes that affect reference sizes; - different object alignment modes that affect object sizes; - different compilation modes: interpreter, C1, C2; - special paths like carrying special bits in layout helper for allocation slow-paths; I know that test is sensitive to compiler intrinsics bugs, as I used these tests to develop the intrinsics. If you inject simple off-by-one bugs into current C1/C2 intrinsics, new test catches that. The additional safety comes from the somewhat loose API requirement: it is specified to return the guess, and that guess might as well be wrong (not overly wrong though, for a quality implementation). ------------- PR: https://git.openjdk.java.net/jdk/pull/650
