RFR (S): 8027295: Free CSet takes ~50% of young pause time

[Stefan Karlsson]

On 2014-02-25 14:51, Thomas Schatzl wrote:
> Hi Stefan,
>
> On Mon, 2014-02-24 at 14:15 +0100, Stefan Karlsson wrote:
>> Hi Thomas,
>>
>> Not a full review, but a comment below:
>>
>> On 2014-02-24 12:40, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have reviews for the remaining parts of 8027295 after splitting
>>> out code cache remembered set data structure changes (JDK-8025406)?
>>>
>>> It only includes several smaller changes around CSet freeing that
>>> improve performance.
>>>
>>> Note that this change is based on 8035406 also out for review
>>>
>>> Changes:
>>>
>>> - fast card cache changes
>>>     - pad FCC rows to cache line size to avoid any false sharing (every
>>> row represents the card cache for a single worker thread)
>> Is it really enough to just pad the size and not the start address of
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8027295
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8027295/webrev.2/
> Fixed in
>
> http://cr.openjdk.java.net/~tschatzl/8027295/webrev.3/

The padding part of this change looks good, consider it reviewed if you 
push that as a separate RFE.

I have a couple of style comments:

http://cr.openjdk.java.net/~tschatzl/8027295/webrev.3/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp.udiff.html

+  // Pad rows to cache line sizes.
+  static const int ints_per_cache_line = DEFAULT_CACHE_LINE_SIZE / sizeof(int);
    int n_par_rs = HeapRegionRemSet::num_par_rem_sets();
+  _from_card_cache_max_regions = align_size_up(max_regions, ints_per_cache_line);
+
+  // Need to make sure that the arrays are aligned too. Allocate a suitably
+  // large array to be able to align its base address later. We do not store
+  // the old pointer as we never free that memory.
+  size_t num_ints_to_alloc = n_par_rs * _from_card_cache_max_regions + ints_per_cache_line - 1;
+  int* data = NEW_C_HEAP_ARRAY(int, num_ints_to_alloc, mtGC);
+  data = (int*) (align_size_up_((uintptr_t)data, DEFAULT_CACHE_LINE_SIZE));

Could you move the "int n_par_rs = " line down. It's sort of hidden 
between the setup of ints_per_cache_line and the first usage of 
ints_per_cache_line.

Could you add a comment that you do this padding to avoid false sharing?

It would have been nice to be able to use the code in padded.hpp. See 
PaddedArray::create_unfreeable(). Do you think it would be worth 
extracting the padding code to a new class for arrays of arrays with 
primitives?

thanks,
StefanK

>
> Sorry.
>
> Thanks,
> Thomas
>