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.

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

Tracepoint #508

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

Tracepoint #508

wants to merge 8 commits into from

Conversation

lievenhey
Copy link
Contributor

Show tracepoints in TimeLineWidget

@lievenhey lievenhey force-pushed the tracepoint branch 2 times, most recently from d54b806 to 5e1dd33 Compare September 21, 2023 12:43
@lievenhey
Copy link
Contributor Author

image

src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.cpp Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the tracepoint branch 3 times, most recently from e481527 to 2b03816 Compare September 29, 2023 17:11
src/models/timelinedelegate.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.h Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Show resolved Hide resolved
src/models/eventmodel.h Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_models.cpp Show resolved Hide resolved
@lievenhey lievenhey force-pushed the tracepoint branch 2 times, most recently from 13acd7e to 1daf271 Compare October 2, 2023 09:17
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

there are still no unit tests for any of the new functionality - we need both! i.e. tests for the new behavior of showing tracepoint events in the model, and tests for the favorite.

we also cannot yet undo a favorite action, i.e. remove a favorite

src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/timelinedelegate.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the tracepoint branch 4 times, most recently from 505cf1e to 150e3e3 Compare October 4, 2023 11:04
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/eventmodel.cpp Outdated Show resolved Hide resolved
src/models/timelinedelegate.cpp Outdated Show resolved Hide resolved
src/models/eventmodelproxy.cpp Show resolved Hide resolved
src/models/eventmodelproxy.h Outdated Show resolved Hide resolved
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

hey @lievenhey - I cleaned up the remaining nits and played around with this. It definitely goes into the right direction.

but there is more work remaining to make this actually useful:

the biggest issue is that all tracepoints for all threads and processes are lumped together into a single row - that is pretty useless.

the way I see it, our "Event Sources" should become a combo box that allows multi-selection. Then, each of these event sources will get one row below the CPU or Thread group.

The proxy should then flatten this, if just a single cost is selected - i.e. such that we keep the view as-is for data files that don't contain any extra tracepoint data.

Right now with your proposed patch here, one can have "Event Source: " and then we'd see the per-thread tracepoints and then one more row with all tracepoints clumped together 🤷 that's not useful

additionally, the tooltips shown in the event delegate are wrong when hovering a tracepoint event, because it only looks at the events of the selected source type, meaning tracepoints are never considered.

once those things are fixed, I am OK with merging this work as a stepping stone. the following steps will then be:

  • time measurement between enter/exit similar to off-CPU time
  • somehow making sure that the favorite rows are always shown, independent of the vertical current scroll position - potentially through a second view?

@lievenhey lievenhey force-pushed the tracepoint branch 3 times, most recently from 0506b95 to 52dc89b Compare October 5, 2023 10:51
@lievenhey
Copy link
Contributor Author

somehow making sure that the favorite rows are always shown, independent of the vertical current scroll position - potentially through a second view?

using a second view is most likely the easiest option since QWidgets don't have stuff like qmls header or footer

@milianw
Copy link
Member

milianw commented Oct 7, 2023

somehow making sure that the favorite rows are always shown, independent of the vertical current scroll position - potentially through a second view?

using a second view is most likely the easiest option since QWidgets don't have stuff like qmls header or footer

Agreed, we just need to sync the views and show just a single header somehow. But this should/ could be done in a follow-up patch and doesn't necessarily block integration of the work here as long as the other important issues are resolved.

@GitMensch
Copy link
Contributor

Is this still in a draft state? It seems that new testcases are now available (but failing) are there still missing ones?

@lievenhey
Copy link
Contributor Author

@milianw I finally found some time to work in this. Can you check if it to your satisfaction?

@lievenhey lievenhey requested a review from milianw November 28, 2023 14:12
@lievenhey lievenhey force-pushed the tracepoint branch 2 times, most recently from 4fafd04 to 8b63f7d Compare November 28, 2023 14:15
@lievenhey lievenhey force-pushed the tracepoint branch 2 times, most recently from 9fa7c07 to a05cc6d Compare November 28, 2023 14:23
@milianw
Copy link
Member

milianw commented Dec 10, 2023

are you sure you pushed the correct work? I don't think you attended the issue I raised above, namely:

the biggest issue is that all tracepoints for all threads and processes are lumped together into a single row - that is pretty useless.

I just checked out your branch, rebased it on master and ran it on data I obtained with

perf record -z --call-graph dwarf -e cycles -e raw_syscalls:sys_enter -e raw_syscalls:sys_exit ./tests/test-clients/cpp-parallel/cpp-parallel

The result is still a single list of tracepoints across all threads, which is not useful. What's worse, some threads are suddenly showing up multiple times in the timelines above?

Screenshot_20231210_124332

@lievenhey lievenhey force-pushed the tracepoint branch 2 times, most recently from 48c9f49 to e80f5f6 Compare January 15, 2024 15:02
@lievenhey
Copy link
Contributor Author

Ok, fixed that. Now all events are shown in one line (before there was one line per even type).
That is still somewhat wip since now it is really hard to differentiate different event types since only the tooltip reveals the type. I propose the following:

  • different colors for different event types
  • when hovering highlight not only all stacks but also all symbols with the same type

lievenhey added a commit that referenced this pull request Sep 17, 2024
Since we use QSortFilterProxyModel we get this for free.
This patch adds support for regex search by not escaping the search term
if it is prefix with %.
For the flamegraph there is a custom implementation, which changes the
current QString::contains to a QRegularExpression::match call.

Closes: #508
@lievenhey lievenhey force-pushed the tracepoint branch 5 times, most recently from 89f14ee to 88417d5 Compare November 19, 2024 10:20
The tracepoints are getting a better visual representation on the
timelinewidget so this is no longer necessary.
Move the tracepoints from TimeAxisHeaderView to TimeLineWidget so that
we can use the header for cpu usage.
This also improves usability since the tracepoints are no longer bundles
in one line. They now each have their own line.
Enum automatically counts up so there is no need to manually set these
values.
this allows the user to group important timelines together so that he
can compare them better
lievenhey and others added 3 commits November 19, 2024 11:31
The favourites and tracepoint patches include some rows in the model
that may be empty. To keep the code simple an readable all rows will be
shown. Then a proxy model is put ontop to remove empty rows.
This way we can more easily find them and changing the sort order
doesn't move them to the bottom.
Showing only one cost is fine if we only show a hardware event, but
since we now support tracepoints and some come in an enter/exit pair it
requires us to rework the timeline delegate.
This patch makes the event source combobox multi select and allows to
select multiple event sources.
@lievenhey
Copy link
Contributor Author

Currently it looks like this:
grafik

@lievenhey
Copy link
Contributor Author

waiting for KDAB/perfparser#37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants