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.

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

8207200: Committed > max memory usage when getting MemoryUsage #2281

Closed

Conversation

luchenlin
Copy link
Contributor

@luchenlin luchenlin commented Nov 20, 2023

I backport this for parity with 11.0.22-oracle.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • JDK-8207200 needs maintainer approval
  • JDK-8209061 needs maintainer approval
  • JDK-8209062 needs maintainer approval

Issues

  • JDK-8207200: Committed > max memory usage when getting MemoryUsage (Bug - P3)
  • JDK-8209061: Move G1 serviceability functionality to G1MonitoringSupport (Enhancement - P4)
  • JDK-8209062: Clean up G1MonitoringSupport (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2281/head:pull/2281
$ git checkout pull/2281

Update a local copy of the PR:
$ git checkout pull/2281
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2281/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2281

View PR using the GUI difftool:
$ git pr show -t 2281

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2281.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 20, 2023

👋 Welcome back andrewlu! 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 openjdk bot changed the title Backport db7b4e20e1f75f6b78c8827aa5f8ccfd842f01ab 8207200: Committed > max memory usage when getting MemoryUsage Nov 20, 2023
@openjdk
Copy link

openjdk bot commented Nov 20, 2023

This backport pull request has now been updated with issue and summary from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Nov 20, 2023
@luchenlin
Copy link
Contributor Author

/issue add JDK-8209061
/issue add JDK-8209062

@openjdk
Copy link

openjdk bot commented Nov 20, 2023

@luchenlin
Adding additional issue to issue list: 8209061: Move G1 serviceability functionality to G1MonitoringSupport.

@openjdk
Copy link

openjdk bot commented Nov 20, 2023

@luchenlin
Adding additional issue to issue list: 8209062: Clean up G1MonitoringSupport.

@mlbridge
Copy link

mlbridge bot commented Nov 20, 2023

Webrevs

@jerboaa
Copy link
Contributor

jerboaa commented Nov 20, 2023

This seems a fairly risky backport for a somewhat minor issue that it tries to solve. Are you sure this one is appropriate for OpenJDK 11u in its current lifecyce? Also, I agree with @RealCLanger on his comment on the bug and what he said, hasn't changed IMO.

@luchenlin luchenlin closed this Nov 22, 2023
@GoeLin
Copy link
Member

GoeLin commented Nov 30, 2023

Hi @jerboaa
There are a few points to this.
First, I don't think the issue is that minor. As I understand it, the MemoryUsage reported is always bogus since the values can be updated concurrently. It only became apparent when committed exceeded max.
Further, two things have changed since 2020:
Oracle obviously considers the risk acceptable.
OpenJDK falls back behind Oracle JDK and reports wrong values.

Finally, others asked for this, too: Man Cao in https://bugs.openjdk.org/browse/JDK-8209061

If you agree, I would ask Andrew to backport the four changes involved individually and ask Richard, our GC expert, to review them.
What do you think?
(https://bugs.openjdk.org/browse/JDK-8208498, https://bugs.openjdk.org/browse/JDK-8209061, https://bugs.openjdk.org/browse/JDK-8209062, https://bugs.openjdk.org/browse/JDK-8207200)

@jerboaa
Copy link
Contributor

jerboaa commented Nov 30, 2023

First, I don't think the issue is that minor. As I understand it, the MemoryUsage reported is always bogus since the values can be updated concurrently.

I guess it depends. Let me try to understand this better. It's still a reporting issue, is it not? Is it affecting regular GC operation?

@GoeLin
Copy link
Member

GoeLin commented Dec 4, 2023

Is it affecting regular GC operation?

8208498 might alter the regions touched by a GC.
8207200 adds locking, so it can affect timing.

@jerboaa
Copy link
Contributor

jerboaa commented Dec 4, 2023

Let me try to rephrase. You said: [...] the issue isn't that minor. Why? Is it affecting regular GC operation? If so in what way and why is that important enough to fix now? It has to outweigh the benefits of accepting the risk to backport those enhancements to an old codebase. I'm arguing, if it's affecting reporting only, it's not justified to backport (even if Oracle decided to backport it). I've not heard enough arguments in favor of accepting the risk of this backport, but if I do, I might change my mind.

@GoeLin
Copy link
Member

GoeLin commented Dec 7, 2023

With "isn't that minor" I mean that it does not only affect reporting if max is reached, as the bug title proposes, but any reporting which uses the corrupted counters.

But I'm fine with skipping this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
3 participants