RFR (XS): 8036860: Pad and cache-align the BiasedMappedArray

[Stefan Karlsson]

On 2014-03-14 12:10, Thomas Schatzl wrote:
> Hi Stefan,
>
>   thanks for the review!
>
>
> On Fri, 2014-03-14 at 10:51 +0100, Stefan Karlsson wrote:
>> On 2014-03-14 10:35, Thomas Schatzl wrote:
>>> Hi,
>>>
>>>     I messed up the webrev link (and the CR number in the topic) due to
>>> two very similarly named CRs I currently have assigned to me.
>>>
>>> Here is the correct webrev URL:
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8036860/webrev/
>> http://cr.openjdk.java.net/~tschatzl/8036860/webrev/src/share/vm/gc_implementation/g1/g1BiasedArray.hpp.udiff.html
>>
>> Could you move create_new_base_array(size_t length, size_t elem_size) to
>> the .cpp file, so that we don't have to include padded.inline.hpp in the
>> .hpp file?
>>
>> The allocation.inline.hpp include should probably be chanted to
>> allocation.hpp.
> Actually, with the move of create_new_base_array() these include
> statements could be removed completely. And no allocation include file
> is needed any more.
>
>>
>> http://cr.openjdk.java.net/~tschatzl/8036860/webrev/src/share/vm/memory/padded.inline.hpp.udiff.html
>>
>> +  return (T*)align_size_up_((uintptr_t)chunk, alignment);
>>
>> Could you use align_pointer_up instead, as you did in 8035815?
> Done.
>
>>
>> Other than that, this looks good.
> All fixed.
>
> New complete webrev
> http://cr.openjdk.java.net/~tschatzl/8036860/webrev.1/
>
> Diff to last:
> http://cr.openjdk.java.net/~tschatzl/8036860/webrev.1.diff/

Looks good.

StefanK

>
> (Note that in this diff webrev,
> G1BiasedMappedArrayBase::create_new_base_array() shows the comment
> wrongly indented. This has been fixed in the "complete" webrev above
> already. I only noticed that after looking at the diffs for a final
> time, but I had already merged back the changes from your review to the
> patch).
>
> Thanks,
> Thomas
>
>