Execution comparison #13

farajidaneshgar · 2024-03-20T17:10:32Z

execution comparison:
-Introduce a module to compare two groups of executions using a differential flame graph.
-An "EventDensityView" is used to select a time range for each group and Select the desired traces in an experiment.
-Showing the difference ratio based on self time required a modification to AggregatedCalledFunction.
-Introducing "ParametricWeightedTreeUtils" to get "SelfTime" instead of "duration" while building the differential Trees.
-Adding the option of textual input of dates and query to build the differential flame graph.
-moving grouping to hamburger menu
-Differential flame graph view is updated to be able to show and hide Query part by demand.

remove commented code: Done
improve commit messages, no "second commit" and "third commit":Done
fix dependencies:Done
update aggregatecallsite to have functions like getWeight(string). This doesn't compile locally:Done

Dependencies to the incubator callstack still exist and need to be removed: Done
Modifications to the incubator callstack need to be moved to trace compass core profiling plugin.:Done

MatthewKhouzam · 2024-03-21T15:47:59Z

Can you make the build pass?

MatthewKhouzam · 2024-04-24T13:22:47Z

I tried using it yesterday.

The code doesn't build on HEAD. This is because Arnaud deprecated the incubator callstack
There is open tracing changes in the first patch, they should be a separate patch
splitting the code into "first patch" "second patch" and "third patch" is not descriptive enough. We need to have titles that describe what they do.

I have a use case that can use it. I can share the traces, but I cannot even get it to work at the moment!

farajidaneshgar · 2024-04-24T13:26:15Z

Hello Matthew, Yes. I have new push eliminating that dependencies and adding some other features. I will pull it today.

…

On Wed, Apr 24, 2024 at 9:23 AM Matthew Khouzam ***@***.***> wrote: I tried using it yesterday. 1. The code doesn't build on HEAD. This is because Arnaud deprecated the incubator callstack 2. There is open tracing changes in the first patch, they should be a separate patch 3. splitting the code into "first patch" "second patch" and "third patch" is not descriptive enough. We need to have titles that describe what they do. I have a use case that can use it. I can share the traces, but I cannot even get it to work at the moment! — Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJIHYVI6Q6AFGNYQ5TIZQGTY66W35AVCNFSM6AAAAABE74PPIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZUHE2DANZQGQ> . You are receiving this because you authored the thread.Message ID: <eclipse-tracecompass-incubator/org.eclipse.tracecompass. ***@***.***>

arfio · 2024-04-25T12:51:10Z

Just writing this comment to update the situation:

Dependencies to the incubator callstack still exist and need to be removed
Modifications to the incubator callstack need to be moved to trace compass core profiling plugin.

MatthewKhouzam · 2024-04-25T13:13:04Z

remove commented code
improve commit messages, no "second commit" and "third commit"
fix dependencies
update aggregatecallsite to have functions like getWeight(string). This doesn't compile locally

MatthewKhouzam · 2024-04-29T18:25:47Z

please update commit messages, also thanks for fixing the pr!

MatthewKhouzam

Some first pass changes. you need to fix imports too. I am trying to get this working too!

MatthewKhouzam · 2024-05-01T03:56:33Z

...yses/org.eclipse.tracecompass.incubator.executioncomparision.core.tests/META-INF/MANIFEST.MF

+Require-Bundle: org.eclipse.core.runtime,
+ org.eclipse.core.resources,
+ org.eclipse.tracecompass.common.core,
+ org.eclipse.tracecompass.incubator.executioncomparision.core,


why should it be removed? It s needed in Activator class of test package.

MatthewKhouzam · 2024-05-01T03:57:25Z

...cecompass/incubator/internal/executioncomparision/core/DifferentialSeqCallGraphAnalysis.java

+    private ITmfTimestamp fStartB = TmfTimestamp.BIG_BANG;
+    private ITmfTimestamp fEndB = TmfTimestamp.BIG_CRUNCH;
+    private String fStatistic = ""; //$NON-NLS-1$
+    private List<String> fTraceListA = new ArrayList<>();


CopyOnWriteArrayList, it's a racy list. This is a temp fix.

MatthewKhouzam · 2024-05-01T03:57:31Z

...cecompass/incubator/internal/executioncomparision/core/DifferentialSeqCallGraphAnalysis.java

+    private ITmfTimestamp fEndB = TmfTimestamp.BIG_CRUNCH;
+    private String fStatistic = ""; //$NON-NLS-1$
+    private List<String> fTraceListA = new ArrayList<>();
+    private List<String> fTraceListB = new ArrayList<>();


MatthewKhouzam · 2024-05-01T03:58:04Z

...cecompass/incubator/internal/executioncomparision/core/DifferentialSeqCallGraphAnalysis.java

+            WeightedTreeSet<ICallStackSymbol, Object> newTreeSet = new WeightedTreeSet<>();
+            String mainGroup = MERGE;
+
+            for (String traceName : traceList) {


this can be null, if tracelist is null, continue?

this can't be null. In initialization step ( private List fTraceListA = new ArrayList<>(); private List fTraceListB = new ArrayList<>();) traceLists are initialized as a empty list. And they will be filled by the signal handling before calling this function. However if it is null, the function returns an empty weightedTreeSet.

MatthewKhouzam · 2024-05-01T04:00:17Z

analyses/org.eclipse.tracecompass.incubator.executioncomparision.ui/META-INF/MANIFEST.MF

+ org.eclipse.tracecompass.incubator.analysis.core,
+ org.eclipse.tracecompass.incubator.executioncomparision.core,
+ org.eclipse.jdt.annotation;bundle-version="[2.0.0,3.0.0)";resolution:=optional,
+ jakarta.validation.jakarta.validation-api,


MatthewKhouzam · 2024-05-01T04:00:17Z

analyses/org.eclipse.tracecompass.incubator.executioncomparision.ui/META-INF/MANIFEST.MF

+ org.eclipse.tracecompass.incubator.executioncomparision.core,
+ org.eclipse.jdt.annotation;bundle-version="[2.0.0,3.0.0)";resolution:=optional,
+ jakarta.validation.jakarta.validation-api,
+ org.eclipse.tracecompass.analysis.profiling.core


put above annotations

MatthewKhouzam · 2024-05-01T04:00:52Z

...cecompass/incubator/internal/executioncomparision/core/DifferentialSeqCallGraphAnalysis.java

@@ -192,7 +181,7 @@ public IWeightedTreeSet<ICallStackSymbol, Object, DifferentialWeightedTree<ICall
     *            the monitor, can be null
     * @return the differential weighted provider or null
     */
-    public @Nullable DifferentialWeightedTreeProvider<?> getDiffProvider(@Nullable IProgressMonitor monitor) {
+   public @Nullable DifferentialWeightedTreeProvider<?> getDiffProvider(@Nullable IProgressMonitor monitor) {


MatthewKhouzam · 2024-05-01T04:01:38Z

...cecompass/incubator/internal/executioncomparision/core/DifferentialSeqCallGraphAnalysis.java

@@ -360,7 +368,16 @@ public void dispose() {
     */
    private void setDifferentialCallGraphProvider(DifferentialCallGraphProvider differentialCallGraphProvider) {
        fDifferentialCallGraphProvider = Objects.requireNonNull(differentialCallGraphProvider);
-
+    }
+    private static ITmfTrace getTrace(String traceName) {


this needs to be annotated nullable guess what, in github, nullable is a user that gets pinged!! @PatrickTasse

I can't see why, its called in this loop: for (String traceName : traceList) { ITmfTrace trace = getTrace(traceName);} and it can not be null.

ebugden · 2024-05-02T17:13:08Z

Hello! Are there screenshots or a video screen capture that could be used to give feedback on the result without needing to do the whole development setup? My understanding is that building it myself is currently the only way.

arfio · 2024-05-06T19:36:45Z

analyses/org.eclipse.tracecompass.incubator.executioncomparision.core/META-INF/MANIFEST.MF

+ org.eclipse.tracecompass.lttng2.ust.core,
+ org.eclipse.tracecompass.analysis.os.linux.core,
+ org.eclipse.tracecompass.analysis.profiling.core,
+ org.eclipse.tracecompass.incubator.callstack.core,


This line should be removed.

arfio · 2024-05-06T19:37:55Z

analyses/org.eclipse.tracecompass.incubator.executioncomparision.core/META-INF/MANIFEST.MF

+Bundle-ManifestVersion: 2
+Bundle-Name: %Bundle-Name
+Bundle-Vendor: %Bundle-Vendor
+Bundle-SymbolicName: org.eclipse.tracecompass.incubator.executioncomparision.core;singleton:=true


Ideally, a refactor to rename this package to executioncomparison should be done.

arfio · 2024-05-06T19:38:31Z

...tracecompass/incubator/internal/executioncomparision/core/DifferentialCallGraphProvider.java

@@ -0,0 +1,90 @@
+/*******************************************************************************
+ * Copyright (c) 2023 École Polytechnique de Montréal


All of the copyrights may be updated to 2024

arfio · 2024-05-06T19:41:42Z

...core/src/org/eclipse/tracecompass/incubator/internal/executioncomparision/core/Messages.java

Is this file needed ?

arfio · 2024-05-06T19:46:34Z

...compass/incubator/internal/callstack/core/instrumented/callgraph/AbstractCalledFunction.java

This change should be removed from this PR and moved to another if needed.

arfio · 2024-05-06T19:46:41Z

...mpass/incubator/internal/callstack/core/instrumented/callgraph/AggregatedCalledFunction.java

This change should be removed from this PR and moved to another if needed.

ebugden · 2024-05-13T13:43:24Z

Hello! Are there screenshots or a video screen capture that could be used to give feedback on the result without needing to do the whole development setup? My understanding is that building it myself is currently the only way.

Reminder that it would be great to have some screenshots or a video capture showing the feature. It would allow folks who do not have the Trace Compass dev setup to review.

Introduce a module to compare two groups of executions using a differential flame graph. An "EventDensityView" is used to select a time range for each group. Select the desired traces in an experiment. Showing the difference ratio based on self time (it required a modification to AggregatedCalledFunction). Introducing the "ParametricWeightedTreeUtils" to get "SelfTime" instead of "duration" while building the differential Trees. Isolating duplicating flame graph TODO: * add grouping strategies (WALL TIME) Not Valid: It was Discussed for Async Trace Comparison * move grouping to hamburger menu * add progress bar * instrument code (pretty good) * add tests * Make work with mainline (should be a simple ID swap) * Make a registry for tracetype->callstack ID * remove hard disk operations from UI thread (in progress) * Add way to select the entire trace easily: * Add way to manually select precise time range (import from main?) * query will be added Signed-off-by: Fariba Daneshgar <faraji.daneshgar@gmail.com> Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>

Adding the option of textual input of dates and query to build the differential flame graph. Making work with mainline TODO: * move grouping to hamburger menu * add progress bar * instrument code (pretty good) * add tests * Make a registry for tracetype->callstack ID * remove hard disk operations from UI thread (in progress) * Add way to select the entire trace easily Signed-off-by: Fariba Daneshgar <faraji.daneshgar@gmail.com>

Differential flame graph view is updated to be able to show and hide Query part by demand. grouping are added to hamburger menu Dependencies to incubator for callstack and weighted tree packages are reslved changing in time ranges or in query reflect in graphical treeview and density charts Adding the reset Button to select entire trace (back to initial state) TODO * Add progress bar * instrument code(pretty good) * add test * make a registry for traceTYpe->callstack ID Signed-off-by: fariba <faraji.daneshgar@gmail.com>

changes required considering the changes in trace compass core( method getweigth(statistic type) from AggregatedCalledFunction is eliminated. mthode refreshMouseProver from Tmfxychartviewer ie eliminated) paramerticWeigthedTreeUtils is modified to use getSelfTime() and getweigth() instead of get weigth(Statistic type) ExecutionComparisonView and MultipleEventDensityViewer are changed to reflect time ranges selection in event density charts without using the method refreshMouseProvider(). Resolving Race condition Eliminating unnecessary dependencies and other issues Signed-off-by: fariba <faraji.daneshgar@gmail.com>

farajidaneshgar · 2024-05-16T16:13:56Z

Hi ebugden, here is some screenshots:

farajidaneshgar · 2024-05-16T16:15:38Z

ebugden · 2024-05-23T15:45:26Z

Thank you for the screenshots and for your work on this analysis! 🌻 It's practical to be able to see the result visually in order to give feedback.

Some elements I like:

The diverging colour scale! The encoding choice of blue (for better) and red (for worse) is also good for folks who are red/green colour blind.
I think the percentage at the front of the label makes it easier to notice that that's the primary information of the visualisation.
The "Trace name" label (above the trace selection area).

Suggestions

I have some questions and comments about the visualisation.

Clarify how to read the chart

I noticed that there is a label of "GroupB-GroupA" next to the chart, but it's still not clear what data the chart contains. Some questions that came to mind were:

What is being "diff"-ed between Group B and Group A? Since it's a flame graph, is it the difference in percentage of execution time? Is it the difference in execution time?
Which proportions (the rectangles labelled with function names) are displayed in the flame graph? Is it the proportions (distribution of execution time) of Group B? Is it the proportions of Group B that have comparable proportions in Group A?
- How are functions in Group B that are not in Group A displayed? How are functions that are in Group A, but not in Group B displayed?

Suggestions

I think there should be clear documentation somewhere about what data the chart shows and how to read it that answers these questions.
Clarify that the visualisation is a kind of Flame Graph. I think for people who are just discovering this visualisation it would not be possible to know. E.g. Put a label "Flame Graph" somewhere.
Clarify the structure of the visualisation (i.e. that the top section is just data selection, the bottom section is the visualisation). This could be done with labels, but I think it's possible that applying some of the suggested changes below would improve this (e.g. making the histogram data in the graphical selection area all the same colour)

Clarify when this chart should be used

At the moment it's unclear to me what kind of things it is appropriate to compare with this visualisation. I feel that answering the above questions (in the "Clarify how to read the chart" section) would improve this.

Miscellaneous suggestions

I included screenshots annotated with my comments below.

Some important themes:

Clarify axis titles and labels (and remove when not relevant). This will help with clarifying what is being visualised both in the graphical selection and in the main chart.
Make histogram data all the same colour (in the graphical selection area)

ebugden · 2024-05-31T16:33:15Z

Adding comments from an offline discussion.

How to read the chart

Structure: The structure of the Flame Graph is defined exclusively by the Group B selection (i.e. the proportions and names are defined by Group B)
Colour: The colour of the bars is determined by the difference in absolute execution time between Group B and Group A which is then encoded by a divergent (classified?) colour scale (blue: less time than A, red: more time than A).

Comments

I think how the chart is read needs to be further clarified in the chart itself (e.g. clearer labels, legend, textual description)
Arguably proportions are the most important information. However with the current implementation, the longest portions of execution are far to the right and also there is a lot of visual emphasis on work done by B that is not done by A because they show up as dark red and because the largest (which isn't necessarily the most important information).

MatthewKhouzam · 2024-07-23T20:25:09Z

This was merged! :) :) :) 👍 💯

farajidaneshgar force-pushed the execution-comparison branch 3 times, most recently from dfb1b20 to 938a549 Compare April 24, 2024 18:49

arfio requested review from MatthewKhouzam and arfio April 25, 2024 13:42

farajidaneshgar closed this Apr 28, 2024

farajidaneshgar deleted the execution-comparison branch April 28, 2024 22:37

farajidaneshgar restored the execution-comparison branch April 29, 2024 15:39

farajidaneshgar reopened this Apr 29, 2024

farajidaneshgar force-pushed the execution-comparison branch from 938a549 to 8cf63a9 Compare April 29, 2024 15:41

MatthewKhouzam requested changes May 1, 2024

View reviewed changes

farajidaneshgar force-pushed the execution-comparison branch from 8cf63a9 to 2004b40 Compare May 1, 2024 15:55

arfio requested changes May 6, 2024

View reviewed changes

farajidaneshgar added 4 commits May 16, 2024 11:50

farajidaneshgar force-pushed the execution-comparison branch from 2004b40 to bc713dd Compare May 16, 2024 16:05

vladarama mentioned this pull request Jun 14, 2024

Implement Execution Comparison #38

Merged

MatthewKhouzam closed this Jul 23, 2024

		@@ -0,0 +1,90 @@
		/*******************************************************************************
		* Copyright (c) 2023 École Polytechnique de Montréal

Execution comparison #13

Execution comparison #13

Conversation

farajidaneshgar commented Mar 20, 2024 • edited Loading

MatthewKhouzam commented Mar 21, 2024

MatthewKhouzam commented Apr 24, 2024

farajidaneshgar commented Apr 24, 2024 via email

arfio commented Apr 25, 2024

MatthewKhouzam commented Apr 25, 2024 • edited by arfio Loading

MatthewKhouzam commented Apr 29, 2024

MatthewKhouzam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebugden commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebugden commented May 13, 2024

farajidaneshgar commented May 16, 2024

farajidaneshgar commented May 16, 2024

ebugden commented May 23, 2024

Suggestions

Clarify how to read the chart

Suggestions

Clarify when this chart should be used

Miscellaneous suggestions

ebugden commented May 31, 2024

How to read the chart

Comments

MatthewKhouzam commented Jul 23, 2024

farajidaneshgar commented Mar 20, 2024 •

edited

Loading

MatthewKhouzam commented Apr 25, 2024 •

edited by arfio

Loading