RFR: Implement value type buffering for the interpreter
david.simms at oracle.com
Thu Jun 29 13:02:37 UTC 2017
Brilliant, great work on the comments !
Push it !
On 26/06/17 19:11, Frederic Parain wrote:
> Here's a new webrev:
> - fix parameter default value in global.hpp
> - add comment about VTBufferChunk fields in vtBuffer.hpp
> - add two new counters to track failed chunk allocations
> 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
>>> 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?
>>> On 20/06/17 22:50, Frederic Parain wrote:
>>>> Here's a webrev implementing value type buffering for the interpreter.
>>>> 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.
More information about the valhalla-dev