RFR: 8205683: Refactor heap allocation to separate concerns

[Kim Barrett]

> On Jun 27, 2018, at 9:23 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8205683/webrev.02_03/
> 
> Full:
> http://cr.openjdk.java.net/~eosterlund/8205683/webrev.03/

I really like the overall approach.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.hpp
 40   CollectedHeap *const _heap;
 41   Thread * const       _thread;
 42   Klass *const         _klass;

Inconsistent placement of "*"; and it seems more usual in our code
(and C++ more widely) to type-cuddle pointer/reference (like
everywhere else in this file).

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.hpp
 52   MemAllocator(Klass* klass, size_t word_size)
 53     : _heap(Universe::heap()),
 54       _thread(Thread::current()),
 55       _klass(klass),
 56       _word_size(word_size)
 57   { }

It seems like there would be a lot of allocators constructed in
contexts where the heap or thread are already known
(e.g. CollectedHeap::obj_allocate and friends, which can refer to
THREAD), so that having additional constructor overloads would be
useful.  Though that would be nicer with C++11 forwarding
constructors.

[Aside: That is my personal preferred formatting for constructors.]

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.hpp
 66   // Raw memory allocation. This may or may not use TLAB allocations to satisfy the
 67   // allocation. A GC implementation may override this function to satisfy the allocation
 68   // in any way. But the default is to try a TLAB allocation, and otherwise perform
 69   // mem_allocate.
 70   virtual HeapWord* mem_allocate(Allocation& allocation) const;

How would a GC override this function?  I don't see any examples, and
had some trouble coming up with a mechanism / idiom.  My guess is that
the idea is that a collector overrides CollectedHeap::obj_allocate and
friends and uses a different set of Allocator classes?  If so, it
might be worth spelling that out.  If not, then I've completely missed
the intended design for extension here.

[And I wonder how nicely that will work out.  I guess the Shenandoah
folks can complain if it seems overly contorted.]

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.cpp
 47   oop*                _obj;

I think we usually use "obj" for an oop, and use other names for oop*.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.cpp
 88   void set_obj(oop obj) const { *_obj = obj; }

I was wondering why we need this function.  I did track it down
eventually; it's to preserve the oop across a potential safepoint by
the jvmti sampler.  Maybe use a focused protection RAII object rather
than exposing a public setter?

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.hpp
 95     const size_t hs = oopDesc::header_size() + 1;

I think this should be using
objArrayOopDesc::header_size(_klass->element_type()).

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/memAllocator.cpp
272   // 3. Try refilling the TLAB and allocating the object in it.

"3." seems left-over from earlier version of code.

------------------------------------------------------------------------------

There are a number of places (particularly in memAllocator.cpp) where
there are calls related to "optional" features (JVMTI, DTRACE, &etc)
where it seems like there ought to be some conditionalization.  But
the old code didn't have any of that either, so okay for now.

------------------------------------------------------------------------------