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

Implement Execution Comparison #38

Merged
merged 5 commits into from
Jul 18, 2024
Merged

Implement Execution Comparison #38

merged 5 commits into from
Jul 18, 2024

Conversation

vladarama
Copy link
Contributor

This PR is a cleanup based on @farajidaneshgar work on the execution comparison (Original PR).
The cleanup includes fixing Eclipse and SonarLint warnings and includes some general code quality and formatting improvements.

Signed-off-by: Vlad Arama [email protected]

@arfio arfio self-requested a review June 14, 2024 19:45
arfio
arfio requested changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@arfio arfio left a comment

Choose a reason for hiding this comment

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

I just wrote a few comments on DifferentialFlameGraphView, I will do another pass tomorrow. If you have any questions/remarks we can discuss that in the comment threads.

...mpass/incubator/internal/callstack/core/instrumented/callgraph/AggregatedCalledFunction.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
arfio
arfio requested changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@arfio arfio left a comment

Choose a reason for hiding this comment

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

Lots and lots of small comments, most are easy to fix, others require more thinking/design. Also all of the files are contained under "analyses/org.eclipse.tracecompass.incubator.executioncomparision..." These folders should be renamed to correct the typo. You should remove and reimport the files when doing that change so that eclipse does not recreate these folders.
Good luck !

analyses/org.eclipse.tracecompass.incubator.executioncomparision.ui/plugin.xml Outdated Show resolved Hide resolved
analyses/org.eclipse.tracecompass.incubator.executioncomparision/feature.xml Outdated Show resolved Hide resolved
analyses/org.eclipse.tracecompass.incubator.executioncomparision.core/plugin.xml Outdated Show resolved Hide resolved
analyses/org.eclipse.tracecompass.incubator.executioncomparision/feature.xml Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/MultipleEventDensityViewer.java Outdated Show resolved Hide resolved
.../eclipse/tracecompass/incubator/internal/executioncomparison/ui/ExecutionComparisonView.java Outdated Show resolved Hide resolved
.../eclipse/tracecompass/incubator/internal/executioncomparison/ui/ExecutionComparisonView.java Outdated Show resolved Hide resolved
.../eclipse/tracecompass/incubator/internal/executioncomparison/ui/ExecutionComparisonView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
...lipse/tracecompass/incubator/internal/executioncomparison/ui/DifferentialFlameGraphView.java Outdated Show resolved Hide resolved
@vladarama vladarama force-pushed the execution-comparison branch 2 times, most recently from 428e955 to 1851254 Compare June 21, 2024 14:53
@vladarama vladarama requested a review from arfio July 4, 2024 15:04
@vladarama
Copy link
Contributor Author

@arfio
Thanks for the review, I finished addressing your comments.
I left some questions in the Conversation tabs. I would appreciate a second review when you have the time.

@vladarama vladarama force-pushed the execution-comparison branch 2 times, most recently from d012ac8 to ff140ae Compare July 9, 2024 14:48
@vladarama vladarama requested a review from arfio July 12, 2024 18:40
arfio
arfio previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@arfio arfio left a comment

Choose a reason for hiding this comment

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

Thank you for the overhaul ! This change is ready to be merged for me. Some refactors might still be needed in the future to improve code maintainability, specially for the view classes but believe it can be merged in that state.

@vladarama vladarama force-pushed the execution-comparison branch 2 times, most recently from 88db125 to 35666c6 Compare July 16, 2024 13:04
@vladarama
Copy link
Contributor Author

@arfio
Thanks for the review. I squashed my commits, can you reapprove the changes when you get the chance ?

@vladarama vladarama requested a review from arfio July 16, 2024 13:14
arfio
arfio previously approved these changes Jul 16, 2024
View reviewed changes
bhufmann
bhufmann reviewed Jul 17, 2024
View reviewed changes
Copy link
Contributor

@bhufmann bhufmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

When building the incubator RCP, this feature is not included, nor it's part of the incubator update site (to be able to install it from the mainline RCP's Add-on menu. I'm only able to try it using my development environment. Should it be added to the incubator RCP and update site or are there follow-up patches planned for that?

It would be good to have some documentation on how to use it. This can be also part of a follow-up patch.

...yses/org.eclipse.tracecompass.incubator.executioncomparison.ui.swtbot.tests/build.properties Outdated Show resolved Hide resolved
vladarama
vladarama commented Jul 18, 2024
View reviewed changes
Copy link
Contributor Author

@vladarama vladarama left a comment

Choose a reason for hiding this comment

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

@bhufmann @arfio
I think we can merge this patch as is and do separate follow-up patches for the RCP, update site and documentation which would be easier to review than keeping everything in the same patch.

@vladarama vladarama requested review from bhufmann and arfio July 18, 2024 13:33
@farajidaneshgar @vladarama
execution comparison: adding reset button and some modifications
fcab2a6 
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 <[email protected]>
Signed-off-by: Matthew Khouzam <[email protected]>
@farajidaneshgar @vladarama
execution comparison:textual time range selection
7a88788 
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 <[email protected]>
@farajidaneshgar @vladarama
execution comparison:initial commit
e8dccee 
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 <[email protected]>
@farajidaneshgar @vladarama
execution comparison: resolving issues
b46224f 
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 <[email protected]>
@vladarama vladarama force-pushed the execution-comparison branch 2 times, most recently from f0a1fca to 9b7a401 Compare July 18, 2024 15:15
@vladarama
execution-comparison: improve code
bdabdbd 
This commit fixes various types of formatting issues and code warnings
coming from `Eclipse` and `SonarLint`. The code is also refactored in
various places to reduce the complexity of certain methods.

In addition, it also fixes a bug
where the time range panel (from and to) is broken when initially
launching TC.

Finally, it adds clearer labels for the Y-Axis (Event Count instead of Y
Axis).

Signed-off-by: Vlad Arama <[email protected]>
@bhufmann
Copy link
Contributor

@bhufmann @arfio I think we can merge this patch as is and do separate follow-up patches for the RCP, update site and documentation which would be easier to review than keeping everything in the same patch.

Make sense. I'm ok with this. Please include the plugins and feature to the analysis/pom.xml. If it builds successfully, then we can merged.

bhufmann
bhufmann approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot for this.

analyses/pom.xml
<module>org.eclipse.tracecompass.incubator.executioncomparison</module>
<module>org.eclipse.tracecompass.incubator.executioncomparison.core</module>
<module>org.eclipse.tracecompass.incubator.executioncomparison.core.tests</module>
<module>org.eclipse.tracecompass.incubator.executioncomparison.ui</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this in the last updated. With this the modules are built with maven.

@bhufmann bhufmann merged commit a59e53b into eclipse-tracecompass-incubator:master Jul 18, 2024
2 checks passed
@vladarama vladarama deleted the execution-comparison branch July 19, 2024 14:57
@vladarama vladarama restored the execution-comparison branch July 22, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arfio arfio arfio approved these changes

@bhufmann bhufmann bhufmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vladarama @bhufmann @arfio @farajidaneshgar