Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

stiga-huang
Copy link

@stiga-huang stiga-huang commented Jun 20, 2022

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
Copy link
Author

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

@henri-tremblay
Copy link
Contributor

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 #64

@stiga-huang
Copy link
Author

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 stiga-huang changed the title Java 11 compatibility: Fallback to get fields by Unsafe Java 11 compatibility: Try getting fields by Unsafe in ObjectGraphWalker Jun 22, 2022
@stiga-huang
Copy link
Author

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

@chrisdennis
Copy link
Member

chrisdennis commented Jun 22, 2022

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
Copy link
Author

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 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Java 9 support
3 participants