RFR: Implement value type buffering for the interpreter

[David Simms]

Brilliant, great work on the comments !

Push it !

/Mr. Simms


On 26/06/17 19:11, Frederic Parain wrote:
> Here's a new webrev:
>
> http://cr.openjdk.java.net/~fparain/vt-buffering/webrev.02/index.html
>
> Changes:
>
>   - fix parameter default value in global.hpp
>   - add comment about VTBufferChunk fields in vtBuffer.hpp
>   - add two new counters to track failed chunk allocations
>
> Thanks,
>
> Fred
>
> On 06/26/2017 10:40 AM, Frederic Parain wrote:
>> Mr Simms,
>>
>> Thank you for the review.
>> See my answers in-lined below.
>>
>> On 06/26/2017 10:04 AM, David Simms wrote:
>>>
>>> Kudos on the loop testing, like the targeted testing . Good work on 
>>> the static init fixes and testing.
>>>
>>> Minor comments:
>>>
>>>   * globals.hpp:4097 "sizeof(long long)", is at least 64bits, but not
>>>     fixed size: do you mean BytesPerLong or longSize ?
>>
>> Changed to BytesPerLong.
>> Eventually, this parameter would become a per-platform parameter, to
>> take into account the instruction capabilities of each CPU.
>>
>>>   * ValueTypesBufferMaxMemory=128, global fixed limit
>>>       o "doesn't feel right", but I admit to not having a better 
>>> answer.
>>>         Needs more user testing: TODO, we did talk off list about this,
>>>         discussed combination of global + per-thread options.
>>
>> I've delayed this work to push the VT buffering ASAP.
>> Among the TODO list, I'd like to re-write the backend allocator to
>> avoid fragmenting the memory. I'd like to implement the global/local
>> limits we have discussed, but I need more usage data to tune them.
>>
>>>       o Should "Global VTBuffer Pool statistics" include a failure
>>>         statistic to help with performance evaluation/tuning ?
>>
>> Nice suggestion.
>> A global allocating failure counter could help detecting too small
>> global buffer size.
>> In addition, a per-thread allocation failure counter could help
>> detecting unbalanced use of the global buffer memory.
>> I'll implement it this week.
>>
>>>   * vtBuffer.hpp:40 "_index;" - I can guess what all the other fields
>>>     are at a glance, but a short comment here would be nice
>>
>> Each thread builds its own linked list of VTBufferChunk. Having an
>> index in each chunk indicating its position in the list enables
>> optimizations when comparing the addresses of two buffered values
>> (because buffer chunks are not contiguous, addresses cannot be
>> compared directly).
>> I'll add a comment about that.
>>
>>> I was wondering if "InterpreterFrameClosure" changes didn't trigger 
>>> a problem in the ValueOops whitebox testing (frame oop maps)...that 
>>> test feels a little on the fragile side.
>>
>> The modification of the InterpreterFrameClosure changes the way
>> references to values are handled, but the oopmap themselves are
>> not changed. Do you think this would cause an issue?
>>
>> Thanks,
>>
>> Fred
>>
>>>
>>> On 20/06/17 22:50, Frederic Parain 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
>>>
>>>