RFR #2 (S) CR 8015270: @Contended: fix multiple issues in the layout code

[Daniel D. Daugherty]

On 5/26/13 8:36 PM, David Holmes wrote:
> On 26/05/2013 3:08 AM, Aleksey Shipilev wrote:
>> On 05/25/2013 06:13 PM, Coleen Phillimore wrote:
>>>
>>> This code still looks fine.   One comment, not a suggested change or
>>> request to see this again.
>>>
>>> +  // TODO: We add +1 to always allocate non-zero resource arrays; 
>>> we need
>>> +  // to figure out if we still need to do this.
>>>     int* nonstatic_oop_offsets;
>>>     unsigned int* nonstatic_oop_counts;
>>>     unsigned int nonstatic_oop_map_count = 0;
>>> +  unsigned int max_nonstatic_oop_maps  = fac->count[NONSTATIC_OOP] 
>>> + 1;
>>>
>>>     nonstatic_oop_offsets = NEW_RESOURCE_ARRAY_IN_THREAD(
>>> -            THREAD, int, nonstatic_oop_count + 1);
>>> +            THREAD, int, max_nonstatic_oop_maps);
>>>     nonstatic_oop_counts  = NEW_RESOURCE_ARRAY_IN_THREAD(
>>> -            THREAD, unsigned int, nonstatic_oop_count + 1);
>>> +            THREAD, unsigned int, max_nonstatic_oop_maps);
>>>
>>>
>>> I don't think there's anything to do here.  The code makes it clear
>>> because we allocate these arrays unconditionally, even if the count of
>>> nonstatic oops are zero.   NEW_RESOURCE_IN_ARRAY probably doesn't 
>>> like that.
>>
>> Understood. However, Vladimir has a vague memory about that, and the
>> off-the-wall tests are not failing without +1. Given we care about the
>> footprint, it makes a perfect sense to squeeze a few bytes of the
>> residual footprint.
>>
>> But glancing over the HS codebase, I see no usages for N_R_A_I_T with
>> possible zero argument; it might as well mean we just need to document
>> this in the macro comment, so others would have no questions about that.
>
> Tracing through the code if you get to Amalloc and pass zero then I 
> think things will break because it will return the address of the next 
> empty slot. The next call to Amalloc with a non-zero value will have 
> the exact same address returned!

Agreed on the semantics of Amalloc(). I took a quick look at most of the
places that have NEW_RESOURCE and I think there are a few that just
might be passing zero...

Aleksey's "+ 1" code is safer...

Dan


>
> David
> -----
>
>> -Aleksey.
>>
>