RFR: JDK-8205549 JDK-8205698 Support of flattened values in Unsafe

[John Rose]

On Jun 27, 2018, at 2:17 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
> 
> Please review this changeset which adds support for flattened values to the Unsafe API.
> 
> http://cr.openjdk.java.net/~fparain/Unsafe/webrev.02/index.html
> 
> This changeset addresses the following issues:
>  - reading from /writing to a flattenable/flattened field with Unsafe
>  - null-check when writing to a flattenable field
>  - initialization barrier when reading an uninitialized static value field with Unsafe
>  - enforcing values immutability even when using Unsafe
>  - reading from /writing to a value array with Unsafe
>  - null-check when writing to a value array with Unsafe
> 
> Thanks,
> 
> Fred

Impressive work Fred; you have a nice way to doing the impossible.

The existing version of U.putObject doesn't look at metadata, but clearly
the new version has to depend on metadata to see how the container is shaped.

I see how this can work the way you did it, and why it's useful to unblock
prototyping.  But it's not a design that I want to see made permanent.

Container analysis is a delicate and potentially buggy operation which I am
surprised can be hidden safely under U.putObject.  The basic contract of
Unsafe is to force the user to take responsibility for querying the container,
and issue the U.putX that matches the container.  The benefit of this contract
is not only avoiding delicate and buggy operations inside Unsafe, but also
a simpler optimization model.

So, I view this as a work-in-progress, which can unblock existing
code that uses Unsafe without requiring that code to be revamped.

Later, the get/putObject APIs need to be split into versions which explicitly
manage references vs. flat values.  Unsafe-using code that wants full
optimization will want to avoid get/putObject if it has this complicated
stuff going on under the hood.

New API points are needed to let the user work explicitly with the
container shape:

    public Object getValue(Object o, long offset, Class<?> valueType);
    public void putValue(Object o, long offset, Class<?> valueType, Object value);
    public Object getReference(Object o, long offset);
    public void putReference(Object o, long offset, Object reference);

The get/putValue API points would load and store values of the given type
in their native format (flattened).  If you knew a container wasn't flattened
although the type could be, you'd call putReference.

We could also deprecate get/putObject so that users will be forced to
do the container analysis.  An non-deprecated version of your container
analysis should have a more complex-sounding name, such as
getReferenceOrValue.  Or we should just force users to write their
own; it's simple if the hooks are provided (see below).

The valueType operand is there even when the value is also present,

for robustness.  If we derive valueType from value automatically, the
user loses a channel for asserting the container analysis.  You could
get heap-scribbling bugs when the value is of the wrong type.  Oddly
enough, the Unsafe API has a concern with safety:  You have to give
the user a reasonable chance to write safe code using Unsafe.

I think we also want queries which can execute the logic you have written,
directly, and not in the course of reading or writing the heap.  Something
like this would give unsafe users control over factoring the metadata
queries from the access itself:

    public Class<?> objectFieldContainerType(Field f);
    public Field findObjectField(Class<?> objClass, long offset);
    public Object getReferenceOrValue(Object o, long offset) {
      Class<?> ctype = objectFieldContainerType(findObjectField(o.getClass(), offset));
      if (ctype.isValue())  return getValue(o, offset, ctype);
      else return getReference(o, offset);
    }

All that said, for the present review, it looks good as a work in progress.

(If the benefit of overloading get/putObject is small, consider naming
this new logic with a new name like getReferenceOrValue.  But I'm guessing
the overloading trick is a short-term requirement, to avoid cracking open
lots of platform code.)

If we don't have another bug to track Unsafe support for container analysis
of flat fields, let's file one as a followup to this.

— John