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.

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

Jump to bottom

Fixes calculation of average thread blocked time and average thread waited time #118

Merged
merged 3 commits into from
Apr 8, 2022
Merged

Fixes calculation of average thread blocked time and average thread waited time #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0796e6
Enables thread contention monitoring. Fixes calculation of average th…
eirsep Jan 24, 2022
5c78fd1
check thread contention monitoring enabled/disabled via PA setting
eirsep Apr 4, 2022
40cc6b0
remove test generated files
eirsep Apr 6, 2022
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
3 changes: 2 additions & 1 deletion src/main/java/org/opensearch/performanceanalyzer/collectors/OSMetricsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ public void collectMetrics(long startTime) {
SchedMetricsGenerator schedMetricsGenerator = osMetricsGenerator.getSchedMetricsGenerator();
schedMetricsGenerator.addSample();

Map<Long, ThreadList.ThreadState> threadStates = ThreadList.getNativeTidMap();
Map<Long, ThreadList.ThreadState> threadStates =
ThreadList.getNativeTidMap(getThreadContentionMonitoringEnabled());

DiskIOMetricsGenerator diskIOMetricsGenerator =
osMetricsGenerator.getDiskIOMetricsGenerator();
Expand Down
9 changes: 9 additions & 0 deletions ...va/org/opensearch/performanceanalyzer/collectors/PerformanceAnalyzerMetricsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ enum State {
private String collectorName;
protected StringBuilder value;
protected State state;
private boolean threadContentionMonitoringEnabled;

protected PerformanceAnalyzerMetricsCollector(int timeInterval, String collectorName) {
this.timeInterval = timeInterval;
Expand Down Expand Up @@ -92,4 +93,12 @@ public State getState() {
public void setState(State state) {
this.state = state;
}

public void setThreadContentionMonitoringEnabled(boolean enabled) {
this.threadContentionMonitoringEnabled = enabled;
}

public boolean getThreadContentionMonitoringEnabled() {
return threadContentionMonitoringEnabled;
}
}
13 changes: 13 additions & 0 deletions ...java/org/opensearch/performanceanalyzer/collectors/ScheduledMetricCollectorsExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public class ScheduledMetricCollectorsExecutor extends Thread {
private static final int COLLECTOR_THREAD_KEEPALIVE_SECS = 1000;
private final boolean checkFeatureDisabledFlag;
private boolean paEnabled = false;
private boolean threadContentionMonitoringEnabled = false;
private int minTimeIntervalToSleep = Integer.MAX_VALUE;
private Map<PerformanceAnalyzerMetricsCollector, Long> metricsCollectors;

Expand Down Expand Up @@ -52,7 +53,19 @@ public synchronized boolean getEnabled() {
return paEnabled;
}

public synchronized void setThreadContentionMonitoringEnabled(final boolean enabled) {
metricsCollectors
.keySet()
.forEach(collector -> collector.setThreadContentionMonitoringEnabled(enabled));
threadContentionMonitoringEnabled = enabled;
}

private synchronized boolean getThreadContentionMonitoringEnabled() {
return threadContentionMonitoringEnabled;
}

public void addScheduledMetricCollector(PerformanceAnalyzerMetricsCollector task) {
task.setThreadContentionMonitoringEnabled(getThreadContentionMonitoringEnabled());
metricsCollectors.put(task, System.currentTimeMillis() + task.getTimeInterval());
if (task.getTimeInterval() < minTimeIntervalToSleep) {
minTimeIntervalToSleep = task.getTimeInterval();
Expand Down
12 changes: 11 additions & 1 deletion src/main/java/org/opensearch/performanceanalyzer/jvm/ThreadList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,13 @@ public String toString() {
* acquire this lock and move on if we could not get it.
*
* @return A hashmap of threadId to threadState.
* @param threadContentionMonitoringEnabled
*/
public static Map<Long, ThreadState> getNativeTidMap() {
public static Map<Long, ThreadState> getNativeTidMap(
boolean threadContentionMonitoringEnabled) {
if (threadBean.isThreadContentionMonitoringSupported()) {
threadBean.setThreadContentionMonitoringEnabled(threadContentionMonitoringEnabled);
}
Comment on lines +124 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

This check and set happens during every OSMetricsCollector#collectMetrics call which is very frequent. Do we want to set it once during initialization and when there is state change in threadContentionMonitoringEnabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

This check is not very costly
If the flag is already enabled then this short-circuits out of the method without doing anything.

if (contentionMonitoringEnabled != enable) {
               if (enable) {
                   // if reeabled, reset contention time statistics
                   // for all threads
                   resetContentionTimes0(0);
               }

               // update the VM of the state change
               setThreadContentionMonitoringEnabled0(enable);

               contentionMonitoringEnabled = enable;
           }

Also, the OSMetricsCollector is initialized in the PA Plugin. We can't do a set once and update on state change sort of thing from there as ThreadList.getNativeTidMap is a static call and will need to know thread contention monitoring is enabled or not before every run.

if (vmAttachLock.tryLock()) {
try {
// Thread dumps are expensive and therefore we make sure that at least
Expand Down Expand Up @@ -263,6 +268,9 @@ private static void parseThreadInfo(final ThreadInfo info) {
1.0e-3
* (t.blockedTime - oldt.blockedTime)
/ (t.blockedCount - oldt.blockedCount);
} else if (t.blockedCount == oldt.blockedCount && t.blockedTime > oldt.blockedTime) {
t.avgBlockedTime =
1.0e-3 * (t.blockedTime - oldt.blockedTime + oldt.avgBlockedTime);
} else {
CircularLongArray arr = ThreadHistory.blockedTidHistoryMap.get(t.nativeTid);
// NOTE: this is an upper bound
Expand All @@ -275,6 +283,8 @@ private static void parseThreadInfo(final ThreadInfo info) {
1.0e-3
* (t.waitedTime - oldt.waitedTime)
/ (t.waitedCount - oldt.waitedCount);
} else if (t.waitedCount == oldt.waitedCount && t.waitedTime > oldt.waitedTime) {
t.avgWaitedTime = 1.0e-3 * (t.waitedTime - oldt.waitedTime + oldt.avgWaitedTime);
} else {
CircularLongArray arr = ThreadHistory.waitedTidHistoryMap.get(t.nativeTid);
// NOTE: this is an upper bound
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/opensearch/performanceanalyzer/jvm/ThreadListTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ private static void runOnce() throws InterruptedException {
String params[] = new String[0];
while (true) {
ThreadList.runThreadDump(OSGlobals.getPid(), params);
ThreadList.LOGGER.info(ThreadList.getNativeTidMap().values());
ThreadList.LOGGER.info(ThreadList.getNativeTidMap(false).values());

/*GCMetrics.runOnce();
HeapMetrics.runOnce();
Expand Down