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

Add Xgc options for suballocator heap size and quick allocation #19872

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Add Xgc options for suballocator heap size and quick allocation #19872

merged 1 commit into from
Sep 30, 2024

Conversation

ThanHenderson
Copy link
Contributor

@ThanHenderson ThanHenderson commented Jul 18, 2024 •
edited
Loading

  1. Use VMEM_ALLOC_QUICK by default for allocateRegion in allocate_memory32
  2. Adds -Xgc:suballocatorQuickAllocDisable option that disables the default VMEM_ALLOC_QUICK
  3. Adds -Xgc:suballocatorIncrementSize option that replaces the HEAP_SIZE_BYTES macro and controls the heap increment size
  4. Adds sanity.functional tests

Addresses: eclipse-omr/omr#7190
Signed-off-by: Nathan Henderson [email protected]

Depends on: eclipse-omr/omr#7472

@ThanHenderson ThanHenderson mentioned this pull request Jul 18, 2024
Merged
@ThanHenderson ThanHenderson force-pushed the cmdline-subincr branch 3 times, most recently from 79b65b7 to dbe4f2a Compare July 21, 2024 00:28
@ThanHenderson ThanHenderson changed the title WIP Add Xgc options for suballocator heap size and quick allocation Add Xgc options for suballocator heap size and quick allocation Jul 21, 2024
@ThanHenderson ThanHenderson marked this pull request as ready for review July 21, 2024 00:34
@ThanHenderson
Copy link
Contributor Author

@babsingh this is the accompanying OpenJ9 patch for eclipse-omr/omr#7414 that needs to be coordinated with it.

keithc-ca
keithc-ca requested changes Jul 22, 2024
View reviewed changes
runtime/gc_modron_startup/mmparseXgc.cpp Outdated Show resolved Hide resolved
runtime/oti/j9port.h Outdated Show resolved Hide resolved
test/functional/cmdLineTests/gcsuballoctests/gcsuballocquickalloctest.xml Outdated Show resolved Hide resolved
test/functional/cmdLineTests/gcsuballoctests/gcsuballocquickalloctest.xml Outdated Show resolved Hide resolved
test/functional/cmdLineTests/gcsuballoctests/playlist.xml Outdated Show resolved Hide resolved
@dmitripivkine dmitripivkine added comp:gc depends:omr Pull request is dependent on a corresponding change in OMR labels Jul 24, 2024
dmitripivkine
dmitripivkine approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@dmitripivkine dmitripivkine left a comment

Choose a reason for hiding this comment

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

Looks good to me

@ThanHenderson ThanHenderson requested a review from keithc-ca July 25, 2024 15:05
@ThanHenderson ThanHenderson force-pushed the cmdline-subincr branch 2 times, most recently from 13b4d57 to 1ab8d75 Compare July 26, 2024 17:45
@keithc-ca
Copy link
Contributor

Needs to be coordinated with: eclipse/omr#7414

I think the OMR change can safely be merged without this, or did I miss something?

@ThanHenderson ThanHenderson force-pushed the cmdline-subincr branch 2 times, most recently from 4080a98 to c84226e Compare July 29, 2024 21:04
@ThanHenderson
Copy link
Contributor Author

The OMR patch has been merged. We can start the PR builds for this one too.

@dmitripivkine
Copy link
Contributor

dmitripivkine commented Aug 1, 2024 •
edited
Loading

-Xgc: options are documented. Would you please create doc item to add description for new options?
@pshipton FYI

@pshipton
Copy link
Member

pshipton commented Aug 1, 2024

The OMR changes also need to promote, but PR builds can depend on OMR #master.
Currently promotion is blocked. When eclipse-omr/omr#7430 is merged that might unblock it, or we'll see what the next problem is.

@keithc-ca keithc-ca self-requested a review August 12, 2024 15:34
@ThanHenderson
Add Xgc options for suballocator heap size and quick allocation
8ef9f22 
1. Use VMEM_ALLOC_QUICK by default for allocateRegion in
   allocate_memory32
2. Adds -Xgc:suballocatorQuickAllocDisable option that
   disables the default VMEM_ALLOC_QUICK
3. Adds -Xgc:suballocatorIncrementSize option that replaces the
   HEAP_SIZE_BYTES macro and controls the heap increment size
4. Adds sanity.functional tests

Addresses: eclipse-omr/omr#7190
Signed-off-by: Nathan Henderson <[email protected]>
@ThanHenderson
Copy link
Contributor Author

I've removed the ability to propagate the port globals from one port lib to another; we can just rely on default values now specified on the OMR side to ensure that the memoryCheckPortLib has some non-zero value for the suballocatorIncrementSize. We no longer need a coordinated merge. memoryCheckPortLib just will not get the values from the -Xgc options. We can revisit that in the future, and I think the memcheck support should be updated anyways to make it more amenable to proper initialization.

fyi @babsingh

@babsingh
Copy link
Contributor

jenkins line endings check

@pshipton
Copy link
Member

Seems like we should have a doc issue opened for this.
https://github.com/eclipse-openj9/openj9-docs/issues/

@babsingh
Copy link
Contributor

jenkins test sanity zlinux,win jdk8,jdk21 depends eclipse-omr/omr#7472

@ThanHenderson ThanHenderson mentioned this pull request Sep 26, 2024
Closed
babsingh
babsingh approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

These changes LGTM. In the PR builds,

I saw three known issues:

Two undocumented issues were seen:

None of the above failures occur due to this PR's changes. @ThanHenderson Can you please open new issues to document the above two failures (PrimeTest & MicroTime)?

I will merge this PR once the latest OMR acceptance build promotes eclipse-omr/omr#7472: https://openj9-jenkins.osuosl.org/job/Pipeline-OMR-Acceptance.

@ThanHenderson
Copy link
Contributor Author

I was just looking into those now too. I will open the new issues.

@babsingh
Copy link
Contributor

babsingh commented Sep 30, 2024 •
edited
Loading

Merging. eclipse-omr/omr#7472 has promoted i.e. the corresponding changes show in https://github.com/eclipse-openj9/openj9-omr/commits/openj9.

@babsingh babsingh merged commit 4e4330d into eclipse-openj9:master Sep 30, 2024
10 of 15 checks passed
@keithc-ca keithc-ca mentioned this pull request Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca approved these changes

@dmitripivkine dmitripivkine dmitripivkine approved these changes

@babsingh babsingh babsingh approved these changes

Assignees
No one assigned
Labels
comp:gc comp:port depends:doc depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ThanHenderson @keithc-ca @dmitripivkine @pshipton @babsingh