[perf] Introduce ftrace marker
Needs ReviewPublic

Authored by romangg on Aug 12 2019, 3:08 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

Adds a class for ftrace marking program flow of KWin. This used in conjunction
with a tool like GPUvis can be very powerful.

I use it at the moment for analyzing the compositing behavior of KWin on X11.
For this I also apply a patch to the XServer adding the same capability[1].

Besides the class also some marking points have been added. Ftracing can be,
if it has been built into the binary, activated via D-Bus or per env var
KWIN_PERF_FTRACE directly from the start.

[1] https://gitlab.freedesktop.org/xorg/xserver/merge_requests/209


Example ftrace in GPUvis

Diff Detail

Repository
R108 KWin
Branch
ftraceMarker
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18785
Build 18803: arc lint + arc unit
romangg created this revision.Aug 12 2019, 3:08 PM
Restricted Application added a project: KWin. ยท View Herald TranscriptAug 12 2019, 3:08 PM
Restricted Application added a subscriber: kwin. ยท View Herald Transcript
romangg requested review of this revision.Aug 12 2019, 3:08 PM
romangg edited the summary of this revision. (Show Details)Aug 12 2019, 3:12 PM
romangg edited the summary of this revision. (Show Details)Aug 12 2019, 3:17 PM
zzag added a subscriber: zzag.Aug 12 2019, 8:44 PM

I think you could just add FtraceMarker class to KWin core and use it directly.

CMakeLists.txt
308

Not sure whether it has to be on by default. This is kind of development thing.

composite.cpp
29

Wrap it in #ifdef KWIN_BUILD_PERF

apol added a subscriber: apol.Aug 14 2019, 10:42 PM

It would be really cool to have a blog post on how to use this once it's in. ๐Ÿ‘

CMakeLists.txt
308

If disabled is not a big penalty. I'd say it's good that it's on, in case you need to test an already existing binary.

dbusinterface.cpp
203

Add a warning?

I'm looking forward to using this in other projects potentially.
Seems a good tool to make use of.

composite.cpp
29

it's fine here.

perf is the public API wrapper around ftracemarker

helpers/perf/ftrace_marker.cpp
86

This all seems overly complex.

the presence of the file, and the function pointers are denoting the same thing

This code is basically just

void FtraceMarker::print()
{
   if (!m_file) return;
   m_file->write(message.toLatin1());
   m_file->flush();
}

void FtraceMarker::print(ulong ctx, const QString &message)
{
   print(message+ QStringLiteral(" (begin_ctx%").arg(ctx);
}

void FtraceMarker::print(const QString &message, ulong ctx )
{
   print(message+ QStringLiteral(" (end_ctx%").arg(ctx);
}

and you can strip half this class

105

If we ever got a line we can't pass properly we would get an empty string here.
QDir with an empty string returns the PWD, which would be a legitmate path you can write into filling up a user homedir

helpers/perf/ftrace_marker.h
43โ€“44

Using overloads to denote different actions (begin/end) is a bit unusual...especially if a future person wants to add a real overload

org.kde.KWin.xml
51

FYI (if you develop something not just for dev purposes) a DBus method implicitly returns whether it succeeded or failed.

You can have a success reply, or an error reply which contains the reason why a call failed in a codified string and human readable form.

romangg updated this revision to Diff 69729.Nov 14 2019, 9:14 AM

Rebase on master.

romangg updated this revision to Diff 69732.Nov 14 2019, 11:00 AM
romangg marked 3 inline comments as done.
  • Use DBus error message
  • No overloaded API functions
  • Check on empty dir name
romangg added inline comments.Nov 14 2019, 11:01 AM
dbusinterface.cpp
203

I used what David proposed (DBus error message).

helpers/perf/ftrace_marker.cpp
86

My idea with this was that if we distribute ftrace marker calls over the code base I don't want to have an additional if conditional checking at every place if tracing is active or not but instead set function pointers that simply return in case tracing is off.

I assumed such a function call would cost less than evaluating the if-conditional. But my research later showed that this does not need to be the case.

And I know it's kind of mini optimizing anyway, so I guess we can simplify the code anyway.

zzag added a comment.Nov 18 2019, 8:01 AM

This still looks slightly over-engineered. You could put everything in one class and add a few convenience methods to make it a bit easier to use the new class.

helpers/perf/ftrace_marker.h
1

I don't think that directory helpers/ is the right place for the FtraceMarker class since it's not an internal utility. Directory helpers/ contains libexec stuff.

apol added a comment.Apr 2 2020, 11:22 PM

Don't we want this anymore? :/ it looked promising