mmtk-core icon indicating copy to clipboard operation
mmtk-core copied to clipboard

Introduce Bounded and Fixed nurseries

Open k-sareen opened this issue 2 years ago • 10 comments

Introduce Bounded and Fixed nurseries

This commit adds Bounded and Fixed nursery types and changes how the nursery size is set. A Bounded nursery has a lower bound of 2 MB but a variable upper bound (set to be 1 TB on 64-bit by default), whereas a Fixed nursery controls both the upper and lower bound of the nursery and sets them to be the same value. By default, MMTk uses a Bounded nursery. The nursery size and type can be set via command line arguments or environment variables, for example, setting MMTK_NURSERY="Fixed:8192" will create a Fixed nursery of size 8192 bytes.

This commit also changes how minor and major GCs are triggered to be more in line with the Java MMTk.

Note: VM bindings may want to change the ObjectModel::VM_WORST_CASE_COPY_EXPANSION constant depending on the worst case expansion that can occur due to object sizes changing when copied.

k-sareen avatar Jul 26 '22 02:07 k-sareen

I tried adding a timer which only timed the major GCs but it was either still counting some mutator time or it was panicking due to None in the statistics code. Currently I've only added a counter for the number of major GCs

k-sareen avatar Jul 26 '22 02:07 k-sareen

If you think the commit is too big, then let me know and I'll break it up into:

  1. PageResource/HeapLayout changes; and
  2. Bounded and Fixed nursery changes.

k-sareen avatar Jul 26 '22 03:07 k-sareen

Yes sorry -- will document code further :+1:

k-sareen avatar Jul 27 '22 02:07 k-sareen

Done. Please don't merge until I've done a performance evaluation after rebasing. I only have stale results.

k-sareen avatar Jul 27 '22 04:07 k-sareen

Done. Please don't merge until I've done a performance evaluation after rebasing. I only have stale results.

Yeah. I won't merge it right now. I will update the CI script version in this PR as well.

qinsoon avatar Jul 27 '22 04:07 qinsoon

Performance results for GenImmix Performance results for GenCopy

Note I used a very generous/roomy heap factor of 4x min heap. I thought it would showcase the difference more starkly. Mostly the PR sees a performance improvement but GenCopy slows down for a few benchmarks which I believe is due to an increased number of GCs from the changes to how minor/major GCs are triggered (hide h2 in the results to make it more visible).

@qinsoon I think the PR is ready to merge if you think the performance results are acceptable. Let me know if you think I should run a different experiment comparing something else.

k-sareen avatar Jul 28 '22 01:07 k-sareen

That looks good. I have also updated the CI scripts. We can merge this PR.

Update: I will run the binding tests just as a final check.

qinsoon avatar Jul 28 '22 02:07 qinsoon

@k-sareen Can you take a look at the failed test? Just check if it is reproducible, and if it is related with the changes in this pull request.

qinsoon avatar Jul 28 '22 05:07 qinsoon

luindex is known to be broken for the generational plans (at least the new luindex). Likely to do with barriers. See: https://github.com/mmtk/mmtk-openjdk/issues/156. I'll investigate and see why this one failed.

k-sareen avatar Jul 28 '22 05:07 k-sareen

luindex is known to be broken for the generational plans (at least the new luindex). Likely to do with barriers. See: mmtk/mmtk-openjdk#156. I'll investigate and see why this one failed.

It is possible that this is a bug revealed by this PR. I don't think this PR changes any code that may cause segfault. But it changes the GC pattern and that may reveal new bugs.

qinsoon avatar Jul 28 '22 05:07 qinsoon