Request for review: 6976350 G1: deal with fragmentation while copying objects during GC

Bengt Rutisson bengt.rutisson at oracle.com
Wed Feb 27 14:40:27 UTC 2013



Hi Tao,

Thanks for prototyping this!

I have only had time to have a quick look today and I'm not sure I will 
have too much time for this over the next few days. So, sorry for not 
having that much feedback.

But from what I've had time to read I think you are on the right track 
and I think Thomas suggestions look good.

Again, sorry for not being able to help more right now.

Bengt

On 2/27/13 11:49 AM, Thomas Schatzl wrote:
> Hi all,
>
> On Tue, 2013-02-26 at 11:01 -0800, Tao Mao wrote:
>> Hi Bengt,
>>
>> When I was refactoring to get another layer of abstraction, I found
>> there would be many duplicated naming and similar functionality in the
>> existing class G1ParGCAllocBuffer and the new class
>> MultiG1ParGCAllocBuffer, surely, in different abstraction level (e.g.
>> allocate(), words_remaining(), retire(), set_word_size(), etc.).
>>
>> I'd like to know whether we are talking about the same idea. What do you
>> have in your mind to lay out the abstraction?
>>
> Imo this kind of hiding the internals of how the allocation is a good
> idea. It removes clutter from program flow. Ideally you should be able
> to exchange occurences of G1ParGCAllocBuffer with
> MultiG1ParGCAllocBuffer recompile, and everything works again (with
> improved performance of course =)
>
> That could be done by making MultiParGCAllocBuffer some descendent of
> ParGCAllocBuffer, and the MultiG1ParGCAllocBuffer uses instances of
> G1ParGCAllocBuffer. There should be no overlap in functionality.
>
> Unfortunately ParGCAllocBuffer does not have a single virtual method
> that can be cleanly overwritten. So it is necessary to introduce an
> (abstract) base class of that that defines the interface for GC
> allocation buffers.
>
> Something like that:
>
> class GCAllocBuffer : public CHeapObj<mtGC> {
> public:
>    virtual HeapWord* allocate(size_t word_sz) = 0;
>    virtual void  undo_allocation(...) = 0;
>    [..]
> };
>
> class ParGCAllocBuffer : public GCAllocBuffer {
>    // the existing implementation overriding every/most of the base
> classes' things
> };
>
> class G1ParGCAllocBuffer : public ParGCAllocBuffer {
>    // add G1 specific stuff
> };
>
> class G1MultiParGCAllocBuffer : public GCAllocBuffer {
> private:
>    G1ParGCAllocBuffer _buffer_sequence[...]; // for clarity, keep with
> the array proposal for now
> public:
>    // implement the GCAllocBuffer interface using the
> G1ParGCAllocBuffers. There should be no implementation duplication
> };
>
> One other question: how do you know whether exactly two additional
> G1ParGCAllocBuffers are the best solution? Maybe one is just fine?
>
> One other, more flexible, method is to link G1ParGCAllocBuffers together
> using an extra member in G1ParGCAllocBuffer (this is somewhat ugly from
> a design point of view though, but G1ParGCAllocBuffer is mostly a helper
> class for G1MultiParGCAllocBuffers). This will likely also simplify the
> "swap_buffers()" method.
>
> Additionally I think swap_buffers() is an implementation detail which
> should be part of retirement of the G1MultiParGCAllocBuffer.
>
> All imho of course.
>
> I found some oddity (not directly related to this change) in the way how
> overriding is used in G1ParGCAllocBuffer: the set_buf() method of
> G1ParGCAllocBuffer hides (not overrides) set_buf() of ParGCAllocBuffer.
> Is this intentional? The same with retire(). Both methods are not
> virtual but "overridden" anyway.
>
> Regards,
> Thomas
>




More information about the hotspot-gc-dev mailing list