8258917: NativeMemoryTracking is handled by launcher inconsistenly

[alexmenkov]

The fix adds NMT handling for non-java launchers

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8258917: NativeMemoryTracking is handled by launcher inconsistenly

Reviewers

• Zhengyu Gu (@zhengyu123 - Reviewer) ⚠️ Review applies to 53e876e

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2106/head:pull/2106
$ git checkout pull/2106

[bridgekeeper]

👋 Welcome back amenkov! 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.

[openjdk]

@alexmenkov The following labels will be automatically applied to this pull request:

• core-libs
• hotspot-runtime

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.

[alexmenkov]

/label add serviceability

[openjdk]

@alexmenkov
The serviceability label was successfully added.

[alexmenkov]

Added serviceability as serviceability tools use launcher functionality

[mlbridge]

Webrevs

• 02: Full - Incremental (dd6f961)
• 01: Full - Incremental (69c6360)
• 00: Full (53e876e)

[zhengyu123]

Looks good

[openjdk]

@alexmenkov 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:

8258917: NativeMemoryTracking is handled by launcher inconsistenly

Reviewed-by: zgu

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 118 new commits pushed to the master branch:

• cf25383: Merge
• f7b96d3: 8259796: timed CompletableFuture.get may swallow InterruptedException
• a37cd5a: 8259859: Missing metaspace NMT memory tag
• 33dcc00: 8132984: incorrect type for Reference.discovered
• 3edf393: 8259978: PPC64 builds broken after JDK-8258004
• 5d8861b: 8259995: Missing comma to separate years in copyright header
• 5cfb36e: 8259036: Failed JfrVersionSystem invariant when VM built with -fno-elide-constructors
• c0e9c44: 8259962: Shenandoah: task queue statistics is inconsistent after JDK-8255019
• 82adfb3: 8134540: Much nearly duplicated code for PerfMemory support
• a9519c8: 8259924: GitHub actions fail on Linux x86_32 with "Could not configure libc6:i386"
• ... and 108 more: https://git.openjdk.java.net/jdk/compare/e4df2098a823d857792c32f99f00eb9f1fb21040...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[dholmes-ora]

Alex,

This approach results in two scans of the argument list in the IsJavaArgs case. I don't know if we care about startup in the non-Java launchers, but this will likely affect it.

David

[mlbridge]

Mailing list message from David Holmes on serviceability-dev:

On 17/01/2021 10:58 pm, David Holmes wrote:

On Fri, 15 Jan 2021 23:50:16 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

The fix adds NMT handling for non-java launchers

Alex,

This approach results in two scans of the argument list in the IsJavaArgs case. I don't know if we care about startup in the non-Java launchers, but this will likely affect it.

Also, I'm not sure the scanning logic in SetJvmEnvironment is valid for
the IsJavaArgs case. It states:

/*
* Since this must be a VM flag we stop processing once we see
* an argument the launcher would not have processed beyond (such
* as -version or -h), or an argument that indicates the following
* arguments are for the application (i.e. the main class name, or
* the -jar argument).
*/

but the argument rules for other commands are different.

David

[alexmenkov]

@dholmes-ora

Also, I'm not sure the scanning logic in SetJvmEnvironment is valid for
the IsJavaArgs case. It states:

/*

• Since this must be a VM flag we stop processing once we see
• an argument the launcher would not have processed beyond (such
• as -version or -h), or an argument that indicates the following
• arguments are for the application (i.e. the main class name, or
• the -jar argument).
  */

but the argument rules for other commands are different.

yes, you are right.
TranslateApplicationArgs translates all "-J" args.
I updated the fix.
For non-java launchers we don't need to scan java args as we know they don't contain -J-XX:NativeMemoryTracking

[alexmenkov]

@dholmes-ora

This approach results in two scans of the argument list in the IsJavaArgs case. I don't know if we care about startup in the non-Java launchers, but this will likely affect it.

The impact is minimal (cycle through args, check if it starts from the string).
As far as I see to avoid extra scans JLI_Launch code needs to be reordered:
CreateExecutionEnvironment();
if (IsJavaArgs()) {
TranslateApplicationArgs(jargc, jargv, &argc, &argv);
}
ParseArguments(&argc, &argv, &mode, &what, &ret, jrepath);
LoadJavaVM();

And handle NMT arg in ParseArguments

But this change would be much more risky.

[dholmes-ora]

Hi Alex,

I think this is functionally correct now - though the comment you added is confusing to me (see below).

However I remain concerned that this requires processing the arg list twice for non-Java launchers. Is that a startup issue we should be concerned about?

Thanks,
David

[dholmes-ora]

What do you mean by this? The -J args are not "translated" here but later in TranslateApplicationArgs.

For non-Java launchers AFAICS you have to process the entire argv array because you don't know where they may appear in general. So to me the comment should be:

• Other launchers (IsJavaArgs())
• All arguments have to be scanned to see if it is a -J argument

[alexmenkov]

What do you mean by this? The -J args are not "translated" here but later in TranslateApplicationArgs.

I meant that they are translated in TranslateApplicationArgs. Looks like it's not clear. Updated the comment as you suggested.

[alexmenkov]

/integrate

[openjdk]

@alexmenkov Since your change was applied there have been 170 commits pushed to the master branch:

• 7be9113: 8255216: Change _directive->BreakAtCompileOption to env()->break_at_compile()
• 5aca934: 8260304: (se) EPollSelectorImpl wakeup mechanism broken on Linux 32-bit
• 53fecba: 8258805: Japanese characters not entered by mouse click on Windows 10
• a887177: 8246788: ZoneRules invariants can be broken
• 874aef4: 8259707: LDAP channel binding does not work with StartTLS extension
• c5ad713: 8260250: Duplicate check in DebugInformationRecorder::recorders_frozen
• bf5e801: 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
• 0ea5862: 8260053: Optimize Tokens' use of Names
• 18eb6d9: 8255348: NPE in PKIXCertPathValidator event logging code
• a97f3c1: 8258853: Support separate function declaration and definition with ENABLE_IF-based SFINAE
• ... and 160 more: https://git.openjdk.java.net/jdk/compare/e4df2098a823d857792c32f99f00eb9f1fb21040...master

Your commit was automatically rebased without conflicts.

Pushed as commit bdc305e.

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

[mlbridge]

Mailing list message from David Holmes on serviceability-dev:

Alex,

On 20/01/2021 11:47 am, Alex Menkov wrote:

On Tue, 19 Jan 2021 23:16:30 GMT, David Holmes <dholmes at openjdk.org> wrote:

What do you mean by this? The -J args are not "translated" here but later in TranslateApplicationArgs.

I meant that they are translated in TranslateApplicationArgs. Looks like it's not clear. Updated the comment as you suggested.

You integrated this change but neither Zhengyu nor myself approved the
final changes. Zhengyu's Review was prior to the logic fix, and my
comment "this seems functionally correct" was just a comment and we were
still discussing the overhead aspect of this.

David
-----

[alexmenkov]

Hi David,

You integrated this change but neither Zhengyu nor myself approved the
final changes. Zhengyu's Review was prior to the logic fix, and my
comment "this seems functionally correct" was just a comment and we were
still discussing the overhead aspect of this.

David

I was sure I saw 2 approvals for the fix, but most likely I misread it and there were 2 reviewers.
Do you have some suggestions how to handle this?
I don't think it makes sense to revert the change and re-do it from the beginning.
I think we don't need to worry about startup time impact. It's minimal and only non-java launchers are affected (AFAIR the main goal of performance/startup optimizations was java launcher).
I can file an RFE to reorder launcher logic and handle NMT in ParseArguments as I described earlier. If it's possible, this would remove one cycle (SetJvmEnvironment) for java/javaw launchers.

[mlbridge]

Mailing list message from David Holmes on serviceability-dev:

Hi Alex,

On 28/01/2021 7:44 am, Alex Menkov wrote:

On Tue, 19 Jan 2021 23:18:04 GMT, David Holmes <dholmes at openjdk.org> wrote:

Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:

Non-lava launchers should process all "-J" arguments

Hi Alex,

I think this is functionally correct now - though the comment you added is confusing to me (see below).

However I remain concerned that this requires processing the arg list twice for non-Java launchers. Is that a startup issue we should be concerned about?

Thanks,
David

Hi David,

You integrated this change but neither Zhengyu nor myself approved the
final changes. Zhengyu's Review was prior to the logic fix, and my
comment "this seems functionally correct" was just a comment and we were
still discussing the overhead aspect of this.

David

I was sure I saw 2 approvals for the fix, but most likely I misread it and there were 2 reviewers.
Do you have some suggestions how to handle this?

Just ask Zhengyu to add his final Review as a comment to show he is okay
with the final result, and I'll do the same via this email. :)

I don't think it makes sense to revert the change and re-do it from the beginning.

No.

I think we don't need to worry about startup time impact. It's minimal and only non-java launchers are affected (AFAIR the main goal of performance/startup optimizations was java launcher).

Yes, it is our main concern. But that's not to say someone else may not
be concerned. Any way the point was to discuss the overhead in case it
was an issue and we're deciding we think it is not an issue so okay.

I can file an RFE to reorder launcher logic and handle NMT in ParseArguments as I described earlier. If it's possible, this would remove one cycle (SetJvmEnvironment) for java/javaw launchers.

Please do. I don't know if anyone will look at it, but at least it
acknowledges the current approach is not optimial.

Thanks,
David
-----

[zhengyu123]

Sorry, I overlooked some of details. Final change looks fine to me.

[alexmenkov]

@zhengyu123 Thank you for re-reviewing

@dholmes-ora RFE created: JDK-8260675