RFR: 8205683: Refactor heap allocation to separate concerns

[Per Liden]

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?

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?

  73   virtual oop init_obj(HeapWord* mem) const;

Can we call this initialize() instead? To better line up with allocate().

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.


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 }


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.



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()?


cheers,
Per