RFR (M): 8077144: Concurrent mark initialization takes too long

[Kim Barrett]

> On Mar 7, 2016, at 6:33 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Kim,
> 
>  thanks for your comments.

Only a couple of followup responses, inline.

Thanks for filing RFEs for the pre-existing issues.

>> src/share/vm/gc/g1/g1ConcurrentMark.cpp  
>> 2147   ArrayAllocator<BitMap::bm_word_t, mtGC> allocator(false);
>> 
>> The use of ArrayAllocator with false free_in_destructor introduces
>> cleanup problems that are not being handled.  All the new (in this
>> change set) and existing uses of that feature appear to be discarding
>> the information needed to free the allocated memory when done with 
>> it, because they aren't capturing the distinction between malloc'ed 
>> vs. mmap'ed.
>> 
>> And that feature is being used and then handing the memory off to a
>> BitMap, which could itself attempt to free the memory (if resize were
>> to be called, which hopefully never happens in the cases at hand) 
> 
> It does not.
> 
> Originally I had an extension of the BitMap::resize() method that
> automatically handled this. However this introduced more complexity
> there, and potential regressions in behavior, so I created this helper
> method in G1ConcurrentMark.
> 
> I guess I will see to extending BitMap after all.

BitMap contains an ArrayAllocator; if ArrayAllocator provided better
control for how to allocate (e.g. perhaps like my suggested
constructor option for controlling the malloc/mmap boundary), and
BitMap provided constructor arguments or other access to that
allocation control, that would address the issue.

But note also that some time ago Stefan Karlson was working on
restructuring BitMap to better manage the allocation.  I'm not sure
what happened to that.

> You were looking at an old version of the change where this has not
> been fixed. Sorry. The code in the local version looks as follows:
> 
>  size_t dummy;
>  _count_marked_bytes = Padded2DArray<size_t,
> mtGC>::create_unfreeable(_max_worker_id, _g1h->max_regions(), &dummy,
> false);
> 
> This is the only difference to the version in the webrev.

I think better than &dummy would be to pass NULL as that argument.