8253525: Implement getInstanceSize/sizeOf intrinsics

[shipilev]

This is fork off the SizeOf JEP, JDK-8249196. There is already the entry point in JDK that can use the intrinsic like this: Instrumentation.getInstanceSize. Therefore, we can implement the C1/C2 intrinsic now, hook it up to Instrumentation, and let the tools use that fast path today.

With this patch, JOL is able to be close to deepSizeOf implementation from SizeOf JEP.

Example performance improvements for sizing up a custom linked list:

Benchmark                     (size)  Mode  Cnt       Score      Error  Units

# Default
LinkedChainBench.linkedChain       1  avgt    5     705.835 ±    8.051  ns/op
LinkedChainBench.linkedChain      10  avgt    5    3148.874 ±   37.856  ns/op
LinkedChainBench.linkedChain     100  avgt    5   28693.256 ±  142.254  ns/op
LinkedChainBench.linkedChain    1000  avgt    5  290161.590 ± 4594.631  ns/op

# Instrumentation attached, no intrinsics
LinkedChainBench.linkedChain       1  avgt    5    159.659 ±   19.238  ns/op
LinkedChainBench.linkedChain      10  avgt    5    717.659 ±   22.540  ns/op
LinkedChainBench.linkedChain     100  avgt    5   7739.394 ±  111.683  ns/op
LinkedChainBench.linkedChain    1000  avgt    5  80724.238 ± 2887.794  ns/op

# Instrumentation attached, new intrinsics
LinkedChainBench.linkedChain       1  avgt    5     95.254 ±   0.808  ns/op
LinkedChainBench.linkedChain      10  avgt    5    261.564 ±   8.524  ns/op
LinkedChainBench.linkedChain     100  avgt    5   3367.192 ±  21.128  ns/op
LinkedChainBench.linkedChain    1000  avgt    5  34148.851 ± 373.080  ns/op

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Testing

	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

• JDK-8253525: Implement getInstanceSize/sizeOf intrinsics

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 482c2f2
• Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 1b7290a

Download

$ git fetch https://git.openjdk.java.net/jdk pull/650/head:pull/650
$ git checkout pull/650

[bridgekeeper]

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@shipilev The following labels will be automatically applied to this pull request:

• core-libs
• hotspot
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 06: Full - Incremental (89881e9)
• 05: Full - Incremental (1b7290a)
• 04: Full - Incremental (482c2f2)
• 03: Full - Incremental (132f2c5)
• 02: Full - Incremental (6160f6a)
• 01: Full (d744a913f0bb7398d39a83ab2c3ca2e3eb591dd3)
• 00: Full (be722d4301bd137557e8c3c7bf77ab8b2d155186)

[vnkozlov]

Always run graalunit testing with new intrinsics.
You need to adjust Graal test:
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java

[shipilev]

I just ran CONF=linux-x86_64-server-fastdebug make clean run-test TEST=compiler/graalunit without problems even without the CheckGraalIntrinsics.java changes. Does the test actually work? EDIT: Ah, it is problem-listed by JDK-8254785, dang.

[AlanBateman]

/label remove core-libs

[openjdk]

@AlanBateman
The core-libs label was successfully removed.

[shipilev]

@vnkozlov I added the new block in CheckGraalIntrinsics.java, and made sure it passes the entire compiler/graalunit with JDK-8254785 applied.

[vnkozlov]

Good.

[openjdk]

@shipilev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8253525: Implement getInstanceSize/sizeOf intrinsics

Reviewed-by: kvn, sspitsyn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 30 new commits pushed to the master branch:

• ed615e3: 4907798: MEMORY LEAK: javax.swing.plaf.basic.BasicPopupMenuUI$MenuKeyboardHelper
• 362feaa: 8254661: arm32: additional cleanup after fixing SIGSEGV
• 432c387: 8254317: C2: Resource consumption of ConvI2LNode::Ideal() grows exponentially
• 79ac041: 8256025: AArch64: MachCallRuntimeNode::ret_addr_offset() is incorrect for stub calls
• 6e8b862: 8255559: Leak File Descriptors Because of ResolverLocalFilesystem#engineResolveURI()
• 129ff97: 8231599: NPE when loading a preview classfile from a future Java version
• 5181f9c: 7190978: javax/swing/JComponent/7154030/bug7154030.java fails on mac
• 35284e4: 8255916: [macos] javax/swing/JInternalFrame/6647340/bug6647340.java timed out
• 8638cd9: 8255625: AArch64: Implement Base64.encodeBlock accelerator/intrinsic
• 5de99da: 8237495: Java MIDI fails with a dereferenced memory error when asked to send a raw 0xF7
• ... and 20 more: https://git.openjdk.java.net/jdk/compare/01567b51dc5149e906a2cbdf050272fdc647781e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[shipilev]

Thanks for review, @kvn! I would also like a review from someone from serviceability.

/reviewers 2

[openjdk]

@shipilev
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[shipilev]

Friendly reminder.

[sspitsyn]

Hi Aleksey,

I've not looked at the compiler generated code.
The fix looks okay to me.
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?
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.
Also, what tests did you run to make sure no regression is introduced?

Thanks,
Serguei

[shipilev]

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).

[sspitsyn]

Aleksey,

Thank you for the update!
It looks good to me.
One more nit, I forgot to list in my previous comment, is uneeded '()' around comparisons:
+ static final int REF_SIZE = ((compressedOops == null) || (compressedOops == true)) ? 4 : 8;

Thanks,
Serguei

[shipilev]

One more nit, I forgot to list in my previous comment, is uneeded '()' around comparisons:
+ static final int REF_SIZE = ((compressedOops == null) || (compressedOops == true)) ? 4 : 8;

Right. Fixed that. Thanks!

[shipilev]

Thanks @vnkozlov and @sspitsyn!

/integrate

[openjdk]

@shipilev Since your change was applied there have been 61 commits pushed to the master branch:

• ea576dd: 8254887: C2: assert(cl->trip_count() > 0) failed: peeling a fully unrolled loop
• 05b8245: 8256290: javac/lambda/T8031967.java fails with StackOverflowError on x86_32
• c3139ab: 8256220: C1: x86_32 fails with -XX:UseSSE=1 after JDK-8210764 due to mishandled lir_neg
• 1d3d64f: 8255973: Add more logging to debug JDK-8255917
• e32a4ea: 8253820: Save test images and dumps with timestamps from client sanity suite
• dff26a4: 8256063: Module::getPackages on an unnamed module may return packages that are in a named module
• 531c56e: 8256278: Shenandoah: Avoid num of dead callback from weak processor in Shenandoah root verifier
• b5a9c92: 8256244: java/lang/ProcessHandle/PermissionTest.java fails with TestNG 7.1
• 90f9a70: 8255546: Missing coverage for javax.smartcardio.CardPermission and ResponseAPDU
• 1c0b490: 8256201: java/awt/FullScreen/FullscreenWindowProps/FullscreenWindowProps.java failed
• ... and 51 more: https://git.openjdk.java.net/jdk/compare/01567b51dc5149e906a2cbdf050272fdc647781e...master

Your commit was automatically rebased without conflicts.

Pushed as commit b4d0186.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.