Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce Bounded and Fixed nurseries #630

Merged
merged 2 commits into from
Aug 17, 2022
Merged

Introduce Bounded and Fixed nurseries #630

merged 2 commits into from
Aug 17, 2022

Conversation

k-sareen
Copy link
Collaborator

@k-sareen k-sareen commented Jul 26, 2022 •
edited
Loading

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 k-sareen requested a review from qinsoon July 26, 2022 02:44
@k-sareen k-sareen force-pushed the fix/nursery-size-rebased branch from b87805d to cf8dc95 Compare July 26, 2022 02:47
@k-sareen
Copy link
Collaborator Author

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 k-sareen force-pushed the fix/nursery-size-rebased branch 3 times, most recently from e785cd6 to aedc72e Compare July 26, 2022 03:07
@k-sareen
Copy link
Collaborator Author

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.

qinsoon
qinsoon reviewed Jul 27, 2022
View reviewed changes
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The ported new methods need comments. And there are some minor issues.

I will need to update CI scripts for the ignore_system_gc option.

src/plan/generational/global.rs Show resolved Hide resolved
src/plan/generational/global.rs Outdated

use mmtk_macros::PlanTraceObject;

const WORST_CASE_COPY_EXPANSION: f64 = 1.5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need comments on this. https://github.com/JikesRVM/JikesRVM/blob/5072f19761115d987b6ee162f49a03522d36c697/MMTk/src/org/mmtk/plan/generational/Gen.java#L62

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this seems to be VM specific. It depends on the object size increment during copying and the min object size. It could be moved to vm::ObjectModel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Yes I think you're right. In that case is 1.5x correct for OpenJDK and our other VMs? I can add it to vm::ObjectModel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. 1.5 should fine. For OpenJDK, 1 is probably fine (I assume we do not grow object size during copying for OpenJDK). I don't know how much this would affect performance. If it is significant, we may want to make 1 as the default value, and change the value for JikesRVM to 1.5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved it to ObjectModel, but have kept the value to still be 1.5. I think OpenJDK technically doesn't change the size of copied objects, but I'm not certain so I'm being conservative currently.

src/plan/generational/immix/global.rs Show resolved Hide resolved
src/plan/global.rs Show resolved Hide resolved
src/util/heap/freelistpageresource.rs Outdated Show resolved Hide resolved
src/util/heap/monotonepageresource.rs Outdated Show resolved Hide resolved
src/util/options.rs Show resolved Hide resolved
src/util/options.rs Show resolved Hide resolved
src/util/options.rs Outdated Show resolved Hide resolved
src/util/options.rs Outdated Show resolved Hide resolved
@k-sareen
Copy link
Collaborator Author

Yes sorry -- will document code further 👍

@k-sareen k-sareen force-pushed the fix/nursery-size-rebased branch 2 times, most recently from 89078b9 to d6f046c Compare July 27, 2022 04:36
@k-sareen
Copy link
Collaborator Author

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

@qinsoon
Copy link
Member

qinsoon commented Jul 27, 2022

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.

@k-sareen
Copy link
Collaborator Author

k-sareen commented Jul 28, 2022 •
edited
Loading

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.

@qinsoon
Copy link
Member

qinsoon commented Jul 28, 2022 •
edited
Loading

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 qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Jul 28, 2022
@qinsoon
Copy link
Member

qinsoon commented Jul 28, 2022

@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.

@k-sareen
Copy link
Collaborator Author

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.

@qinsoon
Copy link
Member

qinsoon commented Jul 28, 2022

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.

k-sareen and others added 2 commits August 17, 2022 12:28
@k-sareen
Introduce Bounded and Fixed nurseries
3df4dfa 
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.
@qinsoon @k-sareen
Update ci-perf-kit to 0.6.8 (for ignore_system_gc option name change).
5044dee
@k-sareen k-sareen force-pushed the fix/nursery-size-rebased branch from c04055b to 5044dee Compare August 17, 2022 03:23
@k-sareen k-sareen merged commit 8afe96d into mmtk:master Aug 17, 2022
@k-sareen k-sareen mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qinsoon qinsoon qinsoon approved these changes

Assignees
No one assigned
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@k-sareen @qinsoon