On Tue, 10 Nov 2020 12:04:21 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> 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 > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Trim down the iteration count, and use -Xbatch to wait for compilation > - Use proper constant names in the test > - Merge branch 'master' into JDK-8253525-sizeof-intrinsics > - The new intrinsic-related test > - Revert the change to test > - Merge branch 'master' into JDK-8253525-sizeof-intrinsics > - Add new intrinsics to toBeInvestigated list in CheckGraalIntrinsics.java > - 8253525: Implement getInstanceSize/sizeOf intrinsics 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 ------------- Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/650
