From 16f9917a46bdf361d7807836e49f1b6251f6e2cd Mon Sep 17 00:00:00 2001 From: Patrick Tasse Date: Mon, 19 Jun 2023 11:58:56 -0400 Subject: [PATCH] analysis: Fix leaks in call stack tests and analysis waitForCompletion In unit tests that extend CallStackTestBase, the CallGraphAnalysis gets a CompositeHostModel that is never disposed. The models can be disposed in the @After method of the test, however for this to work, the analysis must be completed before the test ends. In CallStackTestBase, the @Before method schedules the CallStackAnalysisStub(InstrumentedCallStackAnalysis) and waits for its completion before executing the test. However, when the InstrumentedCallStackAnalysis completes its execution, it schedules a CallGraphAnalysis and does not wait for its completion. This can cause the model to be created after the test has completed and attempted to dispose the models, and causes a leak. The InstrumentedCallStackAnalysis waitForCompletion methods should additionally wait for completion of the CallGraphAnalysis. However, the CallGraphAnalysis is setup to have the InstrumentedCallStackAnalysis as a dependent analysis, so this now causes a deadlock as each analysis is waiting for the other to complete. Normally an analysis that has dependent analyses will schedule these and wait for completion before executing itself. But in this case it is the InstrumentedCallStackAnalysis that executes to completion before scheduling the CallGraphAnalysis, so the latter does not need to have the former as a dependent analysis. In other tests that do not extend CallStackTestBase, dispose the models in the @After method. Change-Id: I154d9d1eda7ca43740191f7fda34786c53f69b05 Signed-off-by: Patrick Tasse Reviewed-on: https://git.eclipse.org/r/c/tracecompass/org.eclipse.tracecompass/+/202591 Tested-by: Marco Miller Tested-by: Trace Compass Bot Reviewed-by: Marco Miller --- .../core/tests/CallStackTestBase.java | 4 +- ...ggregatedCalledFunctionStatisticsTest.java | 4 +- .../tests/callgraph/AggregationTreeTest.java | 11 +++- .../tests/callgraph/CalledFunctionTest.java | 11 +++- .../core/callgraph/CallGraphAnalysis.java | 60 +++++-------------- .../InstrumentedCallStackAnalysis.java | 20 ++++++- .../ui/swtbot/tests/FlameGraphTest.java | 4 +- 7 files changed, 63 insertions(+), 51 deletions(-) diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/CallStackTestBase.java b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/CallStackTestBase.java index b9342bf168..99a207bda7 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/CallStackTestBase.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/CallStackTestBase.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2016 École Polytechnique de Montréal + * Copyright (c) 2016, 2023 École Polytechnique de Montréal and others * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License 2.0 which @@ -22,6 +22,7 @@ import org.eclipse.tracecompass.internal.analysis.callstack.core.base.ICallStackSymbol; import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.AggregatedCallSite; import org.eclipse.tracecompass.internal.analysis.callstack.core.instrumented.InstrumentedCallStackAnalysis; +import org.eclipse.tracecompass.internal.analysis.callstack.core.model.ModelManager; import org.eclipse.tracecompass.tmf.core.event.TmfEvent; import org.eclipse.tracecompass.tmf.core.exceptions.TmfTraceException; import org.eclipse.tracecompass.tmf.core.signal.TmfTraceOpenedSignal; @@ -83,6 +84,7 @@ public void tearDown() { if (module != null) { module.dispose(); } + ModelManager.disposeModels(); } /** diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregatedCalledFunctionStatisticsTest.java b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregatedCalledFunctionStatisticsTest.java index 2c5c9d7c7d..12a6cbd8aa 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregatedCalledFunctionStatisticsTest.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregatedCalledFunctionStatisticsTest.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2016 Ericsson + * Copyright (c) 2016, 2023 Ericsson * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License 2.0 which @@ -27,6 +27,7 @@ import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.AggregatedCalledFunction; import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.AggregatedCalledFunctionStatistics; import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.ICallGraphProvider; +import org.eclipse.tracecompass.internal.analysis.callstack.core.model.ModelManager; import org.eclipse.tracecompass.statesystem.core.ITmfStateSystemBuilder; import org.eclipse.tracecompass.statesystem.core.StateSystemFactory; import org.eclipse.tracecompass.statesystem.core.backend.IStateHistoryBackend; @@ -107,6 +108,7 @@ public void disposeCga() { if (cga != null) { cga.dispose(); } + ModelManager.disposeModels(); } /** diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregationTreeTest.java b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregationTreeTest.java index aff4c43520..d0b2f18c22 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregationTreeTest.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/AggregationTreeTest.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2016 Ericsson + * Copyright (c) 2016, 2023 Ericsson * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License 2.0 which @@ -32,6 +32,7 @@ import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.AggregatedCalledFunction; import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.CallGraph; import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.ICallGraphProvider; +import org.eclipse.tracecompass.internal.analysis.callstack.core.model.ModelManager; import org.eclipse.tracecompass.statesystem.core.ITmfStateSystemBuilder; import org.eclipse.tracecompass.statesystem.core.StateSystemFactory; import org.eclipse.tracecompass.statesystem.core.backend.IStateHistoryBackend; @@ -138,6 +139,14 @@ public void before() { TmfTraceManager.getInstance().traceOpened(signal); } + /** + * Cleanup models + */ + @After + public void after() { + ModelManager.disposeModels(); + } + /** * Dispose the callgraph analysis that has been set */ diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/CalledFunctionTest.java b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/CalledFunctionTest.java index b739323245..106ba0b0a1 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/CalledFunctionTest.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.core.tests/src/org/eclipse/tracecompass/analysis/callstack/core/tests/callgraph/CalledFunctionTest.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2016 Ericsson + * Copyright (c) 2016, 2023 Ericsson * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License 2.0 which @@ -25,6 +25,7 @@ import org.eclipse.tracecompass.internal.analysis.callstack.core.callgraph.ICalledFunction; import org.eclipse.tracecompass.internal.analysis.callstack.core.model.IHostModel; import org.eclipse.tracecompass.internal.analysis.callstack.core.model.ModelManager; +import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -54,6 +55,14 @@ public void setup() { fHiFixture = hiFixture; } + /** + * Cleanup models + */ + @After + public void after() { + ModelManager.disposeModels(); + } + /** * This is more to make sure that the arguments are OK except for the state * value diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/callgraph/CallGraphAnalysis.java b/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/callgraph/CallGraphAnalysis.java index 1ca3603f13..ab5025e486 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/callgraph/CallGraphAnalysis.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/callgraph/CallGraphAnalysis.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2016 Ericsson + * Copyright (c) 2016, 2023 Ericsson * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License 2.0 which @@ -37,7 +37,6 @@ import org.eclipse.tracecompass.internal.analysis.callstack.core.model.ModelManager; import org.eclipse.tracecompass.internal.analysis.callstack.core.model.ProcessStatusInterval; import org.eclipse.tracecompass.internal.analysis.callstack.core.tree.IWeightedTreeGroupDescriptor; -import org.eclipse.tracecompass.tmf.core.analysis.IAnalysisModule; import org.eclipse.tracecompass.tmf.core.analysis.TmfAbstractAnalysisModule; import org.eclipse.tracecompass.tmf.core.symbols.ISymbolProvider; import org.eclipse.tracecompass.tmf.core.symbols.SymbolProviderManager; @@ -149,11 +148,6 @@ public boolean canExecute(ITmfTrace trace) { return true; } - @Override - protected Iterable getDependentAnalyses() { - return Collections.singleton(fCsProvider); - } - @Override protected boolean executeAnalysis(@Nullable IProgressMonitor monitor) { return executeForRange(fCallGraph, TmfTimeRange.ETERNITY, monitor); @@ -164,30 +158,18 @@ private boolean executeForRange(CallGraph callgraph, TmfTimeRange range, @Nullab if (monitor == null || trace == null) { return false; } - Iterable dependentAnalyses = getDependentAnalyses(); - for (IAnalysisModule module : dependentAnalyses) { - if (!(module instanceof IFlameChartProvider)) { + IFlameChartProvider callstackModule = fCsProvider; + IHostModel model = ModelManager.getModelFor(callstackModule.getHostId()); + + CallStackSeries callstack = callstackModule.getCallStackSeries(); + if (callstack != null) { + long time0 = range.getStartTime().toNanos(); + long time1 = range.getEndTime().toNanos(); + long start = Math.min(time0, time1); + long end = Math.max(time0, time1); + if (!iterateOverCallstackSerie(callstack, model, callgraph, start, end, monitor)) { return false; } - module.schedule(); - } - // TODO: Look at updates while the state system's being built - dependentAnalyses.forEach(t -> t.waitForCompletion(monitor)); - - for (IAnalysisModule module : dependentAnalyses) { - IFlameChartProvider callstackModule = (IFlameChartProvider) module; - IHostModel model = ModelManager.getModelFor(callstackModule.getHostId()); - - CallStackSeries callstack = callstackModule.getCallStackSeries(); - if (callstack != null) { - long time0 = range.getStartTime().toNanos(); - long time1 = range.getEndTime().toNanos(); - long start = Math.min(time0, time1); - long end = Math.max(time0, time1); - if (!iterateOverCallstackSerie(callstack, model, callgraph, start, end, monitor)) { - return false; - } - } } monitor.worked(1); monitor.done(); @@ -298,15 +280,7 @@ private void iterateOverCallstack(ICallStackElement element, CallStack callstack * @return The collection of callstack series */ public @Nullable CallStackSeries getSeries() { - CallStackSeries series = null; - for (IAnalysisModule dependent : getDependentAnalyses()) { - if (!(dependent instanceof IFlameChartProvider)) { - continue; - } - IFlameChartProvider csProvider = (IFlameChartProvider) dependent; - series = csProvider.getCallStackSeries(); - } - return series; + return fCsProvider.getCallStackSeries(); } @Override @@ -327,13 +301,9 @@ public CallGraph getCallGraph() { @Override public Collection getGroupDescriptors() { List descriptors = new ArrayList<>(); - for (IAnalysisModule module : getDependentAnalyses()) { - if (module instanceof IFlameChartProvider) { - CallStackSeries serie = ((IFlameChartProvider) module).getCallStackSeries(); - if (serie != null) { - descriptors.add(serie.getRootGroup()); - } - } + CallStackSeries serie = fCsProvider.getCallStackSeries(); + if (serie != null) { + descriptors.add(serie.getRootGroup()); } return descriptors; } diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/instrumented/InstrumentedCallStackAnalysis.java b/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/instrumented/InstrumentedCallStackAnalysis.java index 15d02666c3..77e7dc6acc 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/instrumented/InstrumentedCallStackAnalysis.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.core/src/org/eclipse/tracecompass/internal/analysis/callstack/core/instrumented/InstrumentedCallStackAnalysis.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2013, 2015 Ericsson + * Copyright (c) 2013, 2023 Ericsson * * All rights reserved. This program and the accompanying materials are * made available under the terms of the Eclipse Public License 2.0 which @@ -159,6 +159,24 @@ protected boolean executeAnalysis(@Nullable IProgressMonitor monitor) { return true; } + @Override + public boolean waitForCompletion() { + boolean waitForCompletion = super.waitForCompletion(); + if (fAutomaticCallgraph) { + waitForCompletion &= fCallGraph.waitForCompletion(); + } + return waitForCompletion; + } + + @Override + public boolean waitForCompletion(IProgressMonitor monitor) { + boolean waitForCompletion = super.waitForCompletion(monitor); + if (fAutomaticCallgraph) { + waitForCompletion &= fCallGraph.waitForCompletion(monitor); + } + return waitForCompletion; + } + /** * Get the patterns for the process, threads and callstack levels in the * state system diff --git a/analysis/org.eclipse.tracecompass.analysis.callstack.ui.swtbot.tests/src/org/eclipse/tracecompass/analysis/callstack/ui/swtbot/tests/FlameGraphTest.java b/analysis/org.eclipse.tracecompass.analysis.callstack.ui.swtbot.tests/src/org/eclipse/tracecompass/analysis/callstack/ui/swtbot/tests/FlameGraphTest.java index d18074d5cc..080dc8bdc7 100644 --- a/analysis/org.eclipse.tracecompass.analysis.callstack.ui.swtbot.tests/src/org/eclipse/tracecompass/analysis/callstack/ui/swtbot/tests/FlameGraphTest.java +++ b/analysis/org.eclipse.tracecompass.analysis.callstack.ui.swtbot.tests/src/org/eclipse/tracecompass/analysis/callstack/ui/swtbot/tests/FlameGraphTest.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2016 Ericsson + * Copyright (c) 2016, 2023 Ericsson * * All rights reserved. This program and the accompanying materials are made * available under the terms of the Eclipse Public License 2.0 which @@ -119,8 +119,10 @@ public void before() { * Clean up after a test, reset the views and reset the states of the * timegraph by pressing reset on all the resets of the legend */ + @Override @After public void after() { + super.after(); // Calling maximize again will minimize FlameGraphView fg = fFg; if (fg != null) {