RFR: Implement value type buffering for the interpreter

[Frederic Parain]

Karen,

Thank you for the review.
My answers are in-lined below.

> On Jun 23, 2017, at 15:48, Karen Kinnear <KAREN.KINNEAR at ORACLE.COM> wrote:
> 
> Frederic,
> 
> This looks excellent, thank you for doing this so carefully. And thank you
> for the cleanups and diagnostic information.
> 
> I had a few minor questions/comments. I do not need to see any updates - feel
> free to check this in with or without changes.
> 
> questions/comments:
> 1. VTBuffer::allocate_vt_chunk
> line 98 comment - could you possibly change "Done with _pool_lock" to
> "Hold _pool_lock"?
> "Done with" could be translated as you have completed using the _pool_lock.

Fixed.

> 
> 2. I could use help understanding the JDK10 change to root walking
> that meant that you needed to store the java_mirror in the mark word.
> Is there a bugid or comments in the source that I could read up on this?
> Is the intent here that it is faster to just read the mirror than to
> follow the valueKlass field to retrieve the mirror? So this is a memory/GC pause
> time tradeoff and today we have that memory available?

The change has been tracked with CR:
https://bugs.openjdk.java.net/browse/JDK-8154580

> It looks like the existing OopClosure is still being passed the reference heap address not 
> the mirror for non-buffered types - or did I read this incorrectly?

Correct, the behavior for heap-allocated values is the same as for objects.

> 
> 3. InterpreterRuntime::vwithfield line 254
>   I didn't understand why this wasn't a CHECK_0)?

I’m not sure about this one. The assembly part of the bytecode is supposed to use
the value to cleanup the stack, but because an exception is thrown, I don’t know
how it is handled (before it was returning zero and nothing broke).
I still have to learn the details of the exception throwing code.

> 
> 4. mutexLocker.cpp, instanceKlass.cpp and oopMapCAche.cpp - these locks changed to not look for safepoint
> Do we need these changes in hs10?

Yes, absolutely, otherwise a GC could happen during a phase of the buffer
recycling where values are being relocated.
I’ve tried this change on a JDK10 repo, run several tests suites, and
seen no failures. I’ve discussed this change with Coleen, and she thinks
it is fine too.

> 5. UninitializedValueFieldsTest
>   Why did you disable -Xcomp ? 
> 
The code to handle uninitialized static field in the JIT was not available when
I’ve generated the webrev. Tobias has just pushed the missing code, so I
can re-enable the -Xcomp case.

Regards,

Fred

> 
>> On Jun 20, 2017, at 4:50 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
>> 
>> Greetings,
>> 
>> Here's a webrev implementing value type buffering for the interpreter.
>> 
>> http://cr.openjdk.java.net/~fparain/vt-buffering/webrev.01/
>> 
>> Buffering allows the interpreter to allocate values in a thread local
>> buffer to avoid Java heap allocations. Memory recycling is performed
>> on method exit and sometimes on backward branches.
>> 
>> Format of buffered values is almost identical to the format of
>> Java heap allocated values, so most code won't see any difference
>> between a buffered value and a not buffered value. The only difference
>> is in the first word of the header. Because of a change in GC closures
>> in JDK10, the first word now stores a reference to the Java mirror
>> of the value class in order to keep it alive. In JDK9, this operation
>> was performed through the klass pointer in the header, but the GC
>> team has removed this closure.
>> 
>> Buffering can be monitored using NMT or a new diagnostic command.
>> 
>> All tests pass and Sergey has already tested the patch with his
>> benchmark (and reported several bugs that are now fixed, thank
>> you Sergey).
>> 
>> Unfortunately, the changeset also includes a number of fixes not
>> related to buffering, like code clean up and access to uninitialized
>> static value fields.
>> 
>> Thanks,
>> 
>> Fred
>