RFR: 8205683: Refactor heap allocation to separate concerns

[Erik Österlund]

Hi Per,

Thank you for reviewing.

Incremental:
http://cr.openjdk.java.net/~eosterlund/8205683/webrev.02_03/

Full:
http://cr.openjdk.java.net/~eosterlund/8205683/webrev.03/

On 2018-06-27 12:43, Per Liden wrote:
> Hi Erik,
>
> On 06/26/2018 05:13 PM, Erik Österlund wrote:
>> Hi,
>>
>> After a bunch of stuff was added to the CollectedHeap object 
>> allocation path, it is starting to look quite ugly. It has function 
>> pointers passed around with arguments to be sent to them, mixing up 
>> allocation, initialization, tracing, sampling and exception handling 
>> all over the place.
>>
>> I propose to improve the situation by having an abstract MemAllocator 
>> stack object own the allocation path, with suitable subclasses, 
>> ObjAllocator, ClassAllocator and ObjArrayAllocator for allocating the 
>> 3 types we support allocating on the heap. Variation points now 
>> simply use virtual calls, and most of the tracing/sampling code can 
>> be moved out from the allocation path. This should make it easier to 
>> understand what is going on here, and the different concerns are more 
>> separated.
>>
>> A collector can override the virtual member functions for allocating 
>> objects, arrays, and classes, and inject their own variation points 
>> into this framework. For example for installing a forwarding pointer 
>> in the cell header required by Brooks pointers, if you are into that 
>> kind of stuff.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8205683
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8205683/webrev.02/
>
> Awesome cleanup Erik! A few comments/questions/suggestions:
>
> src/hotspot/share/gc/shared/collectedHeap.cpp
> ---------------------------------------------
>
> 449 oop CollectedHeap::obj_allocate(Klass* klass, int size, TRAPS) {
> 450   ObjAllocator allocator(klass, size);
> 451   return allocator.allocate();
> 452 }
> 453
> 454 oop CollectedHeap::array_allocate(Klass* klass, int size, int 
> length, bool do_zero, TRAPS) {
> 455   ObjArrayAllocator allocator(klass, size, length, do_zero);
> 456   return allocator.allocate();
> 457 }
> 458
> 459 oop CollectedHeap::class_allocate(Klass* klass, int size, TRAPS) {
> 460   ClassAllocator allocator(klass, size);
> 461   return allocator.allocate();
> 462 }
>
> I doesn't look like the above functions need to take in TRAPS, right?

It is used by callers so they can use the CHECK_NULL macro on the 
callsite. But inside, I don't need it.

> src/hotspot/share/gc/shared/memAllocator.hpp
> --------------------------------------------
>
> 47   HeapWord* allocate_from_tlab(Allocation& allocation) const;
> 48   HeapWord* allocate_from_tlab_slow(Allocation& allocation) const;
> 49   HeapWord* allocate_outside_tlab(Allocation& allocation) const;
>
> Can we call these allocate_inside_tlab/allocate_outside_tlab?

Fixed.

>  73 virtual oop init_obj(HeapWord* mem) const;
>
> Can we call this initialize() instead? To better line up with allocate().

Fixed.

> Further, I'd also like to see initialize() being a pure virtual 
> function and instead do what that function does today in a different 
> function, protected and non-virtual, called finish(). So in the end 
> we'll have:
>
> allocate();
> initialize();
> finish();
>
> I believe such naming/structure would make code like this easier to 
> understand:
> src/hotspot/share/gc/shared/memAllocator.cpp:
> 415   mem_clear(mem);
> 416   java_lang_Class::set_oop_size(mem, (int)_word_size);
> 417   oop obj = finish(mem);
>
> Otherwise, calling init_obj()/initialize() as the last thing here 
> seems a bit odd and suggests the we're re-initializing the object 
> completely.

Fixed.

> src/hotspot/share/gc/shared/memAllocator.cpp
> --------------------------------------------
> ...
> 391   oop obj = MemAllocator::init_obj(mem);
> 392   assert(Universe::is_bootstrapping() || !obj->is_array(), "must 
> not be an array");
> 393   return obj;
> 394 }
> ...
> 405   oop obj = MemAllocator::init_obj(mem);
> 406   assert(obj->is_array(), "must be an array");
> 407   return obj;
> 408 }
> ...
> 417   oop obj = MemAllocator::init_obj(mem);
> 418   assert(Universe::is_bootstrapping() || !obj->is_array(), "must 
> not be an array");
> 419   return obj;
> 420 }
>
> The above asserts look unnecessary. If so, it would be nice to just 
> convert this to:
>
> ...
> 391   return finish(mem);
> 392 }
> ...
> 405   return finish(mem);
> 406 }
> ...
> 417   return finish(mem);
> 418 }

Fixed.

> 200     HeapWord* mem = (HeapWord*)obj;
> 201     size_t size_in_bytes = _allocator._word_size * HeapWordSize;
> 202     ThreadLocalAllocBuffer& tlab = _thread->tlab();
> 203     size_t bytes_since_last = _allocated_outside_tlab ? 0 : 
> tlab.bytes_since_last_sample_point();
> 204     _thread->heap_sampler().check_for_sampling(mem, size_in_bytes, 
> bytes_since_last);
>
> Looks like we can now change the first arg in check_for_sampling() 
> from HeapWord* to oop and avoid the casting above and inside 
> check_for_sampling() itself.

Fixed.

> src/hotspot/share/gc/shared/threadLocalAllocBuffer.hpp
> ------------------------------------------------------
>
> 143   HeapWord* allocate_sampled_object(size_t size);
>
> Can we call this function allocate_sampled() to better match with 
> allocate()?

Not sure where you found this function; I have removed it. The logic was 
instead moved into the allocate_inside_tlab_slow function. One 
alternative I considered is to pass in a bool* to 
ThreadLocalAllocBuffer::allocate() that if not NULL also tries to bump 
the end to its actual end, and returns back whether the end was bumped 
by the sampling logic or not inside of allocate, and have that pointer 
point straight into Allocation::_tlab_end_reset_for_sample. If you 
prefer that style, I can change it to do that instead.

Thanks,
/Erik

>
> cheers,
> Per