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

8296125: Add a command line option to set a refresh rate of the OS cached metrics in Linux #10918

Closed
wants to merge 3 commits into from

Conversation

omikhaltsova
Copy link

@omikhaltsova omikhaltsova commented Oct 31, 2022

I would like to add a new command line product option:
-XX:OsCachedMetricsRefreshRate=value, -
where a value is times per second and is in the range [1; 1000000000].

It substitutes the hardcoded timeout of 20 ms between re-readings of the OS cached metrics introduced in JDK-8232207 and allows to set this timeout (as a refresh rate) by a user at the launch time.

This option will be available only on Linux.

It can be used as followed:
java -XX:OsCachedMetricsRefreshRate=100 MyApp


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
  • Change requires CSR request JDK-8296126 to be approved

Issues

  • JDK-8296125: Add a command line option to set a refresh rate of the OS cached metrics in Linux
  • JDK-8296126: Add +XX:OsCachedMetricsRefreshRate= to set a refresh rate of the cached metrics (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10918/head:pull/10918
$ git checkout pull/10918

Update a local copy of the PR:
$ git checkout pull/10918
$ git pull https://git.openjdk.org/jdk pull/10918/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10918

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10918.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 31, 2022

👋 Welcome back omikhaltcova! 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 added the csr Pull request needs approved CSR before integration label Oct 31, 2022
@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@omikhaltsova The following label will be automatically applied to this pull request:

  • hotspot-runtime

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

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 31, 2022
@omikhaltsova omikhaltsova marked this pull request as ready for review October 31, 2022 17:49
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 31, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 31, 2022

Webrevs

@omikhaltsova
Copy link
Author

TestVerifyGraphEdges.java fails on linux-x86 due to the previous code changes.
The reason of failure is described in JDK-8295936, JDK-8295867.

Comment on lines 187 to 190
julong os::Linux::oscontainer_cache_timeout() {
return NANOSECS_PER_SEC / OsCachedMetricsRefreshRate;
}

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be part of the osContainer API rather than os::Linux?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I moved it from os::Linux to OSContainer.

@tstuefe
Copy link
Member

tstuefe commented Nov 1, 2022

Does this really have to be a product switch? The JBS issue sounds like it is not even clear if polling the metrics is actually the culprit. How about a debug-only switch, or at least a diagnostic product switch?

@omikhaltsova
Copy link
Author

Yes, it'll be better. The option is made DIAGNOSTIC. Thanks!

@omikhaltsova
Copy link
Author

@tstuefe if the option is DIAGNOSTIC then I don't need CSR, right?

@tstuefe
Copy link
Member

tstuefe commented Nov 2, 2022

@tstuefe if the option is DIAGNOSTIC then I don't need CSR, right?

Exactly :)

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Please also remove OSCONTAINER_CACHE_TIMEOUT definition from osContainer_linux.hpp.

Cheers, Thomas

julong OSContainer::oscontainer_cache_timeout() {
return NANOSECS_PER_SEC / OsCachedMetricsRefreshRate;
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the wrapper is needed, I'd just use the switch directly.

Copy link
Author

Choose a reason for hiding this comment

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

I prefer to keep this wrapper instead of using in the code "NANOSECS_PER_SEC / OsCachedMetricsRefreshRate" in order to preserve an easy reading of code and to be closer to the origin name of the replaced macro - OSCONTAINER_CACHE_TIMEOUT.

I decided to use a refresh rate instead of a timeout as a command line option for easier perception/setting of values i.e.: 50 times/s instead of 20000000 ns (= 20 ms)

But if to use a timeout in ns as a command line option then such a wrapper is not needed.

@tstuefe
Copy link
Member

tstuefe commented Nov 2, 2022

Please also remove OSCONTAINER_CACHE_TIMEOUT definition from osContainer_linux.hpp.

Oh never mind, I see you did this already.

@dholmes-ora
Copy link
Member

@tstuefe if the option is DIAGNOSTIC then I don't need CSR, right?

Exactly :)

Avoiding a CSR request is not a reason to make a flag DIAGNOSTIC. A diagnostic flag is something you change to try and debug a problem in a product build, it is not something you always set. If some applications in some environments will need to run permanently with a different refresh rate then this should remain a full-fledged product flag.

@tstuefe
Copy link
Member

tstuefe commented Nov 2, 2022

@tstuefe if the option is DIAGNOSTIC then I don't need CSR, right?

Exactly :)

Avoiding a CSR request is not a reason to make a flag DIAGNOSTIC. A diagnostic flag is something you change to try and debug a problem in a product build, it is not something you always set. If some applications in some environments will need to run permanently with a different refresh rate then this should remain a full-fledged product flag.

Sorry for the smiley then, I did not want to make light of the CSR process. My original intention was not to pollute the command line space with yet another product switch. If this is to be a product switch, maybe we should investigate the root cause for the slow polling of metrics first. Maybe we can fix it. My preference would have been a debug-only switch, tbh.

@omikhaltsova
Copy link
Author

Concerning the type of the option I personally think that it would be better to have a product flag (as it was initially suggested) because as I realised from the comments to JDK-8232207, nobody knows for sure if a 20ms grace time is good enough for all use-cases. With high probability this request will be raised again in some time. A product flag allows not to return to this question any more. Additionally imho this approach is better then the hardcoded macro value and by default a user gets this 20 ms as a timeout anyway (in case this option is not used at a command line at an app launch time).

But a user is not able to provide me with a reproducer and there is no 100% guarantee that this 20 ms timeout is a culprit => maybe it's better to have a diagnostic option for the 1st iteration and to wait for the 2nd user request to have this option as product and only after that to propose CSR and to open PR with request to convert this option to a product one.

@iklam
Copy link
Member

iklam commented Nov 2, 2022

Do you have any benchmark that shows the benefit of this switch? We don't want to add a switch, even a diagnostic one, just on a suspicion.

@iklam
Copy link
Member

iklam commented Nov 2, 2022

Do you have any benchmark that shows the benefit of this switch? We don't want to add a switch, even a diagnostic one, just on a suspicion.

Also, we need to measure the actual cost of reading the OS metrics. As far as I know, it's just parsing a very simple text string. It's hard to imagine that it would cause any difference if you do it 50 times per second vs 10 times per second.

(The original fix, JDK-8232207, reduces the refresh rate from something like 1000 times per second to 50 times per second, which made a real difference).

However, if you delay updating the OS metrics, you will be running with the old metrics for a longer time. For example, if the memory limit has been reduced, the correct behavior might be to shrink the size of some application caches which would result in lower performance. By changing the refresh rate to 100ms, you effectively would use the larger cache for a longer time, so the measured performance would be higher.

@tstuefe
Copy link
Member

tstuefe commented Nov 2, 2022

But a user is not able to provide me with a reproducer and there is no 100% guarantee that this 20 ms timeout is a culprit => maybe it's better to have a diagnostic option for the 1st iteration and to wait for the 2nd user request to have this option as product and only after that to propose CSR and to open PR with request to convert this option to a product one.

Sorry, it seems odd to add a flag to OpenJDK proper just to analyze a customer scenario. We should have a better understanding of this problem before adding switches. Can't you do it downstream, or hand the customer a custom-built VM?

Do you have any benchmark that shows the benefit of this switch? We don't want to add a switch, even a diagnostic one, just on a suspicion.

+1

Also, we need to measure the actual cost of reading the OS metrics. As far as I know, it's just parsing a very simple text string. It's hard to imagine that it would cause any difference if you do it 50 times per second vs 10 times per second.

(The original fix, JDK-8232207, reduces the refresh rate from something like 1000 times per second to 50 times per second, which made a real difference).

However, if you delay updating the OS metrics, you will be running with the old metrics for a longer time. For example, if the memory limit has been reduced, the correct behavior might be to shrink the size of some application caches which would result in lower performance. By changing the refresh rate to 100ms, you effectively would use the larger cache for a longer time, so the measured performance would be higher.

Maybe it is my limited understanding, but I cannot see an application that needs even sub-second reaction time to a changed container memory limit. @jerboaa what do you think?

Also, as I wrote before, if our polling code is still slow we should improve it. But lets measure first.

@jerboaa
Copy link
Contributor

jerboaa commented Nov 3, 2022

Maybe it is my limited understanding, but I cannot see an application that needs even sub-second reaction time to a changed container memory limit. @jerboaa what do you think?

The use-cases I know of don't rely on this hot-update mechanism. Usually, on k8s, pods/containers get re-created (i.e. restarted) on re-deployments. As far as I know, the reason why those metrics are being re-read to begin with is the existence of an docker update command.

With all that said, I'm +1 on trying to understand the problem better that the user reported. Unfortunately JBS is currently in maintenance so I cannot look at the bug :-(

If the user thinks, the metrics parsing code is the culprit this can get tested easily by letting them specify -XX:-UseContainerSupport (and possibly set the desired memory and cpu cores manually using -XX:ActiveProcessorCount=n and -XX:MaxRAM=m).

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 1, 2022

@omikhaltsova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@senecaspurling
Copy link

senecaspurling commented Dec 15, 2022

I see some comments about "polling being slow" so I just want to clarify that the issue is exactly the opposite. The user wants to make the polling slower. With 50 containers running JVM processes on a host, the host (and the containers as well though to a lesser extent) experience performance problems because of lock contention for cgroups from the JVMs in the containers querying cgroups 50 times / second. They don't want those queries to happen more frequently/more quickly. They want to slow them down, have them happen LESS frequently. With -XX:OsCachedMetricsRefreshRate=1, the lowest possible setting here, this slows the timeout down to 1000ms (1 second). They do see some improvement with this but it's not sufficient.

The JDK feature to dynamically create compiler threads is querying the os for available memory to make a decision whether a new thread should be created or not: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/compileBroker.cpp#L995
That will delegate to cgroups in a container and it will compete for the lock. With 50 containerized processes doing this 50 times/second, the contention for cgroups causes performance issues. Running with -XX:-UseDynamicNumberOfCompilerThreads, or disabling support for containers with -XX:-UseContainerSupport, resolves the issue entirely.

@jerboaa
Copy link
Contributor

jerboaa commented Dec 16, 2022

With -XX:OsCachedMetricsRefreshRate=1, the lowest possible setting here, this slows the timeout down to 1000ms (1 second). They do see some improvement with this but it's not sufficient.

So reading the file once a second is not sufficient? I wonder what would be then. Only disabling the dynamic nature entirely, I guess. But that would be similar to what -XX:-UseDynamicNumberOfCompilerThreads would do.

@dholmes-ora
Copy link
Member

dholmes-ora commented Dec 18, 2022

I see some comments about "polling being slow" so I just want to clarify that the issue is exactly the opposite.
With -XX:OsCachedMetricsRefreshRate=1, the lowest possible setting here, this slows the timeout down to 1000ms (1 second). They do see some improvement with this but it's not sufficient.

So this proposed patch is not actually addressing the issue and serves no real purpose.

The JDK feature to dynamically create compiler threads is querying the os for available memory to make a decision whether a new thread should be created or not

So in that case perhaps we need to revisit the policy being used to decide that?

And we should also look at the use of locking in os::available_memory, if contention is the issue. Though I'm surprised that we would consider creating dynamic compiler threads often enough to cause contention on the locks here. That would also suggest it is the locking that is sub-optimal (critical section[s] too long?).

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 16, 2023

@omikhaltsova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@omikhaltsova
Copy link
Author

I've closed this PR as the proposed new command line option didn't help to fix the user's issue, that was checked on the evaluation build, and I still don't have a reproducer to continue farther investigation of this case. The user accepted as a workaround -XX:-UseDynamicNumberOfCompilerThreads. Thanks a lot for reviewing and sorry for taking your time in vain!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csr Pull request needs approved CSR before integration hotspot-runtime hotspot-runtime-dev@openjdk.org rfr Pull request is ready for review
6 participants