👋 Welcome back dev-jonghoonpark! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@dev-jonghoonpark This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8366716: Move SmapsParser from runtime/os/TestTracePageSizes.java into testlib

Co-authored-by: Stefan Johansson <sjohanss@openjdk.org>
Reviewed-by: sjohanss, syan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 449 new commits pushed to the master branch:

• 702179e: 8369488: Update to use jtreg 8.1
• 4786f8b: 8369448: C2 SuperWord: refactor VTransform to do move_unordered_reduction_out_of_loop during VTransform::optimize
• a3ee821: 8369305: Adjust usage of CDS in the boot JDK
• ... and 446 more: https://git.openjdk.org/jdk/compare/5271448b3a013b2e3edcd619a4a3b975b292dae1...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sendaoYan, @kstefanj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@dev-jonghoonpark this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8366716-create-smapsparser-lib
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@dev-jonghoonpark The following labels will be automatically applied to this pull request:

• core-libs
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Webrevs

• 04: Full - Incremental (e82154fd)
• 03: Full - Incremental (57b1dcaf)
• 02: Full - Incremental (873804c5)
• 01: Full (538b4405)
• 00: Full (bff215a1)

I converted this PR to a draft due to changes in #27143

I’ve finished merging the changes and switched the status back to 'ready for review.'

I have confirmed that both TestTracePageSizes and TestTransparentHugePagesHeap pass when tested locally.

Additionally, I verified that TestTransparentHugePagesHeap also passes without any issues when the -XX:+UseCompressedOops option is added, as shown below:

OutputAnalyzer oa = ProcessTools.executeTestJava("-XX:+Use" + args[0] + "GC", "-Xmx128m", "-Xms128m", "-Xlog:pagesize:thp-%p.log", "-XX:+UseTransparentHugePages", "-XX:+UseCompressedOops", VerifyTHPEnabledForHeap.class.getName());

Is this the correct way to verify it?

/label remove core-libs

@AlanBateman
The core-libs label was successfully removed.

Is this the correct way to verify it?

I think @kstefanj , who wrote the RFE and probably intended to implement it, might have ideas on how to verify this.

Is this the correct way to verify it?

The problem we had in testing (JDK-8366980) was that we saw failures when running with -XX:-UseCompressedOops so that should be explicitly verified (-XX:+UseCompressedOops is the default). Adding it to the process launcher in the code works for testing, but to test without altering the code you can also pass it in. If you run the tests through make:

make test TEST=gc/TestTransparentHugePagesHeap.java JTREG="JAVA_OPTIONS=-XX:-UseCompressedOops"

A couple of other comments without doing a proper review is that the new smaps classes should be located along side HugePageConfiguration under lib/os/linux and I think we want to think about how to structure the classes. Maybe something like this:

public class Smaps {
    public static class Parser { ... }
    public static class Range { ... }
}

@kstefanj

I've also tested it using the method that doesn't require altering the code, and it passed successfully.

Now, I'll move the path and refactor the code as you reviewed.

@dev-jonghoonpark Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

I have finished refactoring and completed testing locally.

Found some time to look closer at this. I would prefer if we could clean up the Smaps class even more when moving it to the testlib. We might want to go even further than this:
master...kstefanj:jdk:pull/27273-v2

I felt it was easier to show you my ideas this way rather than just giving comment. Some names might need improving and some more polishing, but what do you think about the general structure here?

@dev-jonghoonpark Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@kstefanj
Sorry for the late reply.

I’ve reviewed the code you wrote, and I think it’s much clearer!
I’ve updated it with your suggested changes and confirmed that everything works fine in testing.

//

Sorry for rebasing — it’s a habit of mine, and I did it without thinking. I’ll be more careful next time.

@dev-jonghoonpark, thanks for addressing my concerns.

I need to look over it again but I'm traveling this week and will have less time than normal for reviews. I'll see if I can find some time, otherwise I will look at this next week.

@kstefanj Only the author (@dev-jonghoonpark) is allowed to issue the contributor command.

@kstefanj Only the author (@dev-jonghoonpark) is allowed to issue the contributor command.

/contributor add @dev-jonghoonpark
/contributor add @kstefanj

@dev-jonghoonpark @dev-jonghoonpark was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

• /contributor add @openjdk-bot
• /contributor add duke
• /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@dev-jonghoonpark
Contributor Stefan Johansson <sjohanss@openjdk.org> successfully added.

/integrate

@dev-jonghoonpark
Your change (at version e82154f) is now ready to be sponsored by a Committer.

/sponsor

Going to push as commit 90cf3a2.
Since your change was applied there have been 449 commits pushed to the master branch:

• 702179e: 8369488: Update to use jtreg 8.1
• 4786f8b: 8369448: C2 SuperWord: refactor VTransform to do move_unordered_reduction_out_of_loop during VTransform::optimize
• a3ee821: 8369305: Adjust usage of CDS in the boot JDK
• ... and 446 more: https://git.openjdk.org/jdk/compare/5271448b3a013b2e3edcd619a4a3b975b292dae1...master

Your commit was automatically rebased without conflicts.

@kstefanj @dev-jonghoonpark Pushed as commit 90cf3a2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.