RFR (L): 8022853: add ability to Unsafe to load and store uncompressed object and Klass references in a compressed environment

[Christian Thalinger]

On Aug 21, 2013, at 1:58 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:

> On 8/21/2013 4:27 PM, Christian Thalinger wrote:
>> On Aug 21, 2013, at 10:55 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>> 
>>> Hi,
>>> 
>>> I've reviewed this change and I don't know how it can work.   You cannot put uncompressed oops in an object when the setting is UseCompressedOops and have garbage collection work.
>> This is not the main use case for these methods.  Sometimes oops are not compressed even when we run with compressed oops as for the Klass::_java_mirror case.  There may be other fields like this but I don't know about them.
> 
> The other fields are subject to change also.  If there is a specific use case, why not add a JVM_blah entry for that use case?   Reading this code it is completely unclear that the objects passed in are actually Hotspot metadata.   We don't export metadata to Java. From this code, there is no checking what this is allowed to do. It's too general purpose, when you have some specific use case in mind.

Would you also say that Unsafe.getAddress is too general purpose because you can read any kind of address?

One thing that you seem to forget here is that today you can write any unsafe code you want using get/put Int/Long/Address.  You name it.  This is no different except that these new methods are doing the right thing when you know what you are passing in, to which field in what configuration.  This is the whole idea about Unsafe; you have to know what you are doing.

> 
>> 
>>>   Similarly you can't compress arbitrary metadata because only Klass* types can be compressed.   This doesn't seem like it would work at all.
>> Currently get/putMetadata only supports StorageMode.UNCOMPRESSED_KLASS and StorageMode.COMPRESSED_KLASS.  We might add other storage modes in the future if we decide to compress other things.
> 
> There is no indication of this from the code.   From the code, these things are oops, when they are not?   We use type checking in C++ and Java is supposed to be even safer.

…but this is unsafe :-)

> 
>> 
>>>   Also, we lay out objects in classFileParser assuming the setting of UseCompressedOops/ClassPointers and allocates space based on that.
>>> 
>>> I don't understand the motivation of this change.   It appears incorrect.
>> The motivation is to enable Java code which is tightly coupled to the VM to read and possibly write such fields.  Right now it is not possible.
> 
> Which fields and why?   Some sort of compiler interface is probably more appropriate where you can ask the JVM with native calls what you need to know.

For example type checking in compiled code.  You need to read Klass* references from the Class to do the type checking.  The Klass* is either compressed or not.  Right now there is no way to read the right value from (unsafe) Java code and we can't call out to the VM (via JNI or another C++ down call) in hot paths.

>>> From reading the bug, it seems that you want to get to mirror from InstanceKlass from Java.   This will break when we move the mirror.
>> As said above, since it's tightly coupled to the VM we will notice very soon when something changes and will change the Java code accordingly.
> 
> I have to believe that there is a better way to do this. Actually, I have ideas how to do this that might also work for the SA.  

Can you share your thoughts?

-- Chris

> The last thing we want to see is another bunch of Java code tightly coupled with the jvm implementation in another place with another implementation.
> 
> Coleen
>> 
>> -- Chris
>> 
>>> Coleen
>>> 
>>> On 8/19/2013 5:28 PM, Christian Thalinger wrote:
>>>> http://cr.openjdk.java.net/~twisti/8022853/webrev/
>>>> http://cr.openjdk.java.net/~twisti/8022853-jdk/webrev/
>>>> 
>>>> 8022853: add ability to Unsafe to load and store uncompressed object and Klass references in a compressed environment
>>>> Reviewed-by:
>>>> 
>>>> In some native data structures of the VM oops are stored uncompressed when compressed oops are turned on, for example Klass::_java_mirror. Java code which is tightly coupled with the VM should have the ability to read and write such fields.
>>>> 
>