ehcache / sizeof

Configurable sizeOf engine for Ehcache
Apache License 2.0
105 stars 43 forks source link

Java 11 compatibility: Try getting fields by Unsafe in ObjectGraphWalker #65

Closed stiga-huang closed 2 years ago

stiga-huang commented 2 years ago

Currently, ObjectGraphWalker uses reflection to access private fields of a class. When running on Java11, this could fail if the access is denied (java.lang.reflect.InaccessibleObjectException), which results in underestimation.

This PR aims to resolve #52 without workarounds. ObjectGraphWalker is extended to use Unsafe when it fails to set a field accessable.

Tested on the following JDKs:

openjdk version "11.0.2" 2019-01-15 OpenJDK Runtime Environment 18.9 (build 11.0.2+9) OpenJDK 64-Bit Server VM 18.9 (build 11.0.2+9, mixed mode)

java version "11.0.8" 2020-07-14 LTS Java(TM) SE Runtime Environment 18.9 (build 11.0.8+10-LTS) Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.8+10-LTS, mixed mode)

openjdk version "11.0.15" 2022-04-19 LTS OpenJDK Runtime Environment Zulu11.56+19-CA (build 11.0.15+10-LTS) OpenJDK 64-Bit Server VM Zulu11.56+19-CA (build 11.0.15+10-LTS, mixed mode)

stiga-huang commented 2 years ago

@chrisdennis @henri-tremblay @akomakom Could you help to review this?

henri-tremblay commented 2 years ago

It's a bit strange. Normally we fallback to Reflection when unsafe doesn't work. So when would we need to use that? Also, there is this PR that should address the same thing in a more complicated way https://github.com/ehcache/sizeof/pull/64

stiga-huang commented 2 years ago

We need this when run with --illegal-access=deny and without the workaround of adding -Djdk.attach.allowAttachSelf=true. Based on the Java 11 migration guide, --illegal-access=deny will be the default in a future java release. So we are migrating our program to be able to run with it (IMPALA-11260).

I just played around with #64. It still requires -Djdk.attach.allowAttachSelf=true. Even if I added this flag, I still see InaccessibleObjectException thrown in ObjectGraphWalker.getAllFields(). I might miss something. Please let me know if there are specific options needed.

It's a bit strange. Normally we fallback to Reflection when unsafe doesn't work.

I think we do this in estimating shallow sizes. But for getting field values (in ObjectGraphWalker), unsafe is a choice that we should try. JOL does the same: https://github.com/openjdk/jol/blob/5ec187a6d38443604221a63abec39c545d4d1674/jol-core/src/main/java/org/openjdk/jol/util/ObjectUtils.java#L118-L168

I can modify the code to try unsafe first. Just put it later to be consistent with the existing behavior.

stiga-huang commented 2 years ago

Modified the title to make it more appropriate. We only try unsafe when it's available.

chrisdennis commented 2 years ago

I understand the issue here... and the slight confusion too. There are two things going on here:

  1. There is existing 'fallback' code for sizing individual objects in the graph: agent, unsafe, reflection. This PR doesn't touch that.
  2. There is no existing fallback code for the reflection required to 'walk' the graph itself (before passing the individual objects to the sizing code). That is what is being addressed here.

There were some issues here though with how these changes interacted with the soft field caches. I therefore took the idea of using unsafe to navigate the references but implemented it in a different way in #64. @stiga-huang if you'd like to test that the updated version of that code works for you that would be very helpful. In the meantime I'll see what I can do to get that PR pushed through. The only real remaining issue is to decide if we want to directly use the byte-buddy agent, org instead improve our own loading code to be similarly capable.

stiga-huang commented 2 years ago

Can we merge this for 0.4.1 and leave #64 for 0.5.0? We are waiting for a workable solution. I think we still need some time to review #64 .