RFR: 8204088: Dynamic Max Memory Limit

[Thomas Schatzl]

Hi,

On Tue, 2018-06-19 at 20:46 +0200, Rodrigo Bruno wrote:
> Hi all,
> 
> here is the first version of our contribution for draft JEP-8204088. 
> 
> More details at the CR.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8204088
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/jelastic/cmx/
> 
> Thanks,
> Rodrigo 

  this looks even less change that I imagined :)

Some thoughts on the changes:

- the flag is called CurrentMaxCapacity and the corresponding getter in
the code is called "max_current_capacity()". It would be nice to be
uniform in where the "current" should be :)

- please move the checks to prevent expansion into G1's actual heap
memory manager, i.e. HeapRegionManager.

  - the change in G1CollectedHeap::humongous_obj_allocate() would be
automatically subsumed by
HeapRegionManager::find_contiguous_empty_or_unavailable() if it checked
the amount of current region + requested regions <= current max regions
first.
I am actually not sure what the change actually prevents - both
capacity() and max_capacity() do not check CurrentMaxCapacity. Maybe
this should be s/max_capacity()/max_current_capacity()/ ?

  - G1CollectedHeap::resize_if_necessary_after_full_collection() should
probably just use for G1CollectedHeap::max_current_capacity().

  - the change in G1CollectedHeap::expand() will disappear because
is_maximal_no_gc() will automatically adapt (need to fix the
implementation of HeapRegionManager::is_available()).

  - the existing implementation of
G1CollectedHeap::max_current_capacity() should probably just defer to
the HeapRegionManager then.

  - the problem is that now there is an inconsistency with
G1CollectedHeap::is_maximal_no_gc() and CurrentMaxHeapSize. I.e.
is_maximal_no_gc() uses the number of available regions compared to the
absolute maximum number of regions, and not the current maximum number
of regions.

- contrary to Per I am not sure this feature should be done as a
collector specific change.

It would also be a pity to have the flag have a "G1" prefix only to see
other collectors pick this up fairly quickly; the usefulness of such an
option beyond just G1 has already been shown in that other VMs (e.g.
J9) already have the same feature.
This would just mean having two options that do exactly the same thing,
and getting rid of managable flags is not easy.

- there is some issue with the java.lang.Runtime.maxMemory() API which
may need adjustment: in the change that method returns the current
maximum amount of memory the JVM will use, not the absolute maximum.
It may confuse applications which use this function to get different
values depending on when it's called in the future, and might actually
need an update of the specification in that regard.

Does somebody happen to know what J9 reports here if you change current
heap size?

- I need to think more about potential issues with allowing to change
and use the flag (and dependent methods) without other synchronization.

Consider changing this and different threads picking up different
values of the current max heap size (in the VM; the Java application is
another problem).

Very conservatively I would suggest to have a safepoint for that. Also
I think the entry point to the VM for setting manageable flags into the
VM is the SetVMFlagDCmd which may need to be adapted to allow more code
to be run (more below).

Otoh from what I saw the current uses of the CurrentMaxHeapSize flag
and HeapRegionManager are already under the Heap_lock, but that needs
more careful further review.

- I am not sure how the current implementation satisfies this condition
from the JEP: "If it is not possible to increase or decrease the amount
of memory available to the application, the operation should fail. The
user should be informed of the result of the operation." at this time.

As far as I understand it reports back to the VM that it could write
the flag to the new value, but not that the memory management (GC)
accepted that value.

Options could be to have the flags data structure enhanced with a VM
callback like the constraint function (or one could maybe somehow
misuse the constraints function for that).

I guess the runtime team (in the hotspot-runtime-dev mailing list)
would be the people to ask how to best do that if they do not read this
:)

An alternative to the manageable flag would be to use a separate Jmap
("GC.set_current_max_heapsize") command that could do anything already.

- Also I think at the moment any collector would accept changing
CurrentMaxHeapSize at runtime, happily obliging I guess.

- in G1 at least heap sizes must be multiples of region size. This
needs to be checked every time it changes (playing into the custom
callback issues).
(The existing flags will be automatically aligned in
CollectorPolicy::initialize_flags()).
I think overriding CollectorPolicy::initialize_flags() in
G1CollectorPolicy can handle that.

- please fix the indentation in the constraint function for
CurrentMaxHeapSize. We use two spaces indent and it's all over the
place (particularly closing braces) there :)

- the CurrentMaxHeapSize flag should move to the other *HeapSize flags
at least into gc_globals.hpp, not globals.hpp. (May need to move to
g1_globals.hpp pending the discussion whether this should be a g1
specific flag or not).

- there is no test of this functionality. There should be a test which
programmatically tries to update current heap size. This value could be
checked (crudely) by extending WB_PrintHeapSizes() in whitebox.cpp and
then checked like the  tests in
test/hotspot/jtreg/gc/arguments/Test*HeapSizeFlags.java tests do.

That's all for my initial comments.

Thanks,
  Thomas