Skip to content

[hotfix] make previousCpuTicks and previousProcCpuTicks volatile to increase visibility for all threads #26257

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[hotfix] make previousCpuTicks and previousProcCpuTicks volatile to increase visibility for all threads #26257

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71cc401
made previousCpuTicks volatile
bjornarjatten Mar 5, 2025
529badb
make previousProcCpuTicks volatile
bjornarjatten Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...k-runtime/src/main/java/org/apache/flink/runtime/metrics/util/SystemResourcesCounter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ public class SystemResourcesCounter extends Thread {

private volatile boolean running = true;

private long[] previousCpuTicks;
private long[][] previousProcCpuTicks;
private volatile long[] previousCpuTicks;
Copy link
Contributor

@davidradl davidradl Mar 6, 2025

Choose a reason for hiding this comment

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

I had a look at the usages of these 2 fields
it seems previousCpuTicks is only updated if it is null. Which looks very strange - is this right? I can see that volatile could be useful here if this is correct processing.

previousProcCpuTicks is updated when null then after processing in the method see code here. I wonder if the
previousCpuTicks logic should update it, in the same way as previousProcCpuTicks .

I was looking at these usages to see if AtomicReference should be used rather than volatile. This would be more thread safe, and to use compareAndSet() to update it.

Copy link
Author

@bjornarjatten bjornarjatten Mar 20, 2025

Choose a reason for hiding this comment

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

It seems to me that it is also the only place it can be initialized.

I agree with AtomicRefrence, I was thinking about the same thing. Should I add an commit with that? :)

private volatile long[][] previousProcCpuTicks;
private long[] bytesReceivedPerInterface;
private long[] bytesSentPerInterface;

Expand Down