RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no collector specific code.

[Thomas Schatzl]

Hi,

On Sat, 2018-06-16 at 21:00 +0200, Thomas Stüfe wrote:
> Hi Vinay!
> 
> this is the third thread you opened for this issue; it would helpful
> if you would not change subjects, because it splinters discussions on
> the mailing list.

I agree. It is very hard to follow who mentioned what suggestion
already.

It would also help to answer inline as now it is very hard to see who
answered what to which issue, but I think you repeated all points
below.

> ---
> 
> Thank you for your perseverance and patience.
> 
> But unfortunately, my concerns have not been alleviated a lot by the
> current patch. I still think this stretches an already
> partly-ill-defined interface further.
> 
> About os::commit_memory(): as I wrote in my first mail:
> 
> "So far, for the most part, the os::{reserve|commit}_memory APIs have
> been agnostic to the underlying implementation. You pretty much tie
> it to mmap() now. This adds implicit restrictions to the API we did
> not have before (e.g. will not  work if platform uses SysV shm APIs
> to implement these APIs)."
> 
> In your response
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-June/02235
> 6.html
> you explain why you do this. I understand your motives. I still
> dislike this, for the reasons I gave before: it adds a generic API to
> a set of generic APIs which IMHO breaks the implicit contract they
> all have with each other.
> 
> Your patch also blurrs the difference between runtime "os::" layer
> and GC. "os" is a general OS-wrapping API layer, and should not know
> nor care about GC internals:
> 
> +  static inline int nvdimm_fd() {
> +    // ParallelOldGC adaptive sizing requires nvdimm fd.
> +    return _nvdimm_fd;
> +  }
> +  static inline address dram_heapbase() {
> +    return _dram_heap_base;
> +  }
> +  static inline address nvdimm_heapbase() {
> +    return _nvdimm_heap_base;
> +  }
> +  static inline uint nvdimm_regionlength() {
> +    return _nvdimm_region_length;
> +  }
> 
> IMHO, currently the memory management is ill prepared for your patch;
> yes, one could shove it in, but at the expense of maintainability and
> code clearness. This expense would have to be carried by all JVM
> developers, regardless whether they work on your hardware and benefit
> from this feature.
> 
> So I think this would work better with some preparatory refactoring
> done in the VM. Red Hat and Oracle did similar efforts by refactoring
> the GC interface before adding new GCs: see
> https://bugs.openjdk.java.net/browse/JDK-8163329.
> 
> Maybe we could think about how to do this. It certainly would be a
> worthy goal.

  I generally agree with all these sentiments: much of the nvdimm
specific parts should not be in OS nor in the base VirtualSpace class.

Having looked a bit at the code (currently it is a bit hard to get
spare time, this is the reason for the late answer) I also think that
the existing VirtualSpace (and ReservedSpace) code is too much
cluttered up.
Part of this is that the existing change to have the entire heap backed
by one file used the _special flag in ReservedSpace (which *probably*
should be called something like _reservation_already_commits) for its
purposes.

One suggestion that I had when reading the code was that maybe it would
be useful to have VirtualSpace subclasses that handle one particular
way of committing memory within: e.g. one doing nothing (for cases when
the reserved space is precommitted, e.g. SHM page based memory), one
using a regular backing file (I agree that piggy-backing on _special in
ReservedSpace was not a good idea; it would probably very similar to
the no-op one except for constructor/destructor), and another one
handling nvdimm (because apparently compared to a regular backing file
it needs special exit code handling) and one that roughly does what the
current one does, i.e. allowing commit of "lower", "middle" and "high"
part using different page sizes.

Further, then let the user of a ReservedSpace (eg. the collector)
select what VirtualSpace(s) it uses (via e.g. a factory method that
takes the ReservedSpace so that it can return the no-op VirtualSpace in
case everything is already committed).

In case of collectors, they can do more sophisticated selection of the
type of VirtualSpace they need; and apart from G1 they already use
separate VirtualSpaces per generation, so there should be minimal
tweaking to support different VirtualSpaces than now for them.

Looking at Parallel GC, it already owns two different PSVirtualSpaces
that have a very similar interface to VirtualSpace; the PSVirtualSpace
used may already be easily replaced by this "NVDIMMVirtualSpace"
implementation holding all NVDIMM specific information (at the moment I
am not sure why there is a PSVirtualSpace actually, from a
functionality POV it looks very similar to the existing VirtualSpace).

G1 still needs to be changed to properly handle two VirtualSpaces; for
G1 I suggest to use a subclass of HeapRegionManager that simply uses
two VirtualSpaces instead of one; the NVDIMMVirtualSpace where you
internally in HeapRegionManager ignore commits and uncommits, and the
existing one for the remainder.

Then the actual changes to existing code would be limited to, depending
on the command line flag, select one or the other VirtualSpace
(Parallel) or HeapRegionmanager (G1).

I do not see that incorporating the noaccess prefix or the executable
property of ReservedSpace gives too much complication: the former is to
be ignored for the VirtualSpace and completely transparent to them, and
the latter just some flag to pass through).

Forgive me if above is completely outrageously stupid, but that would
probably be what I would start with when standing before this problem.

Thanks,
  Thomas