D23114: [perf] Introduce ftrace marker

[David Edmundson]

davidedmundson added a comment.


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

INLINE COMMENTS

> zzag wrote in composite.cpp:28
> Wrap it in #ifdef KWIN_BUILD_PERF

it's fine here.

perf is the public API wrapper around ftracemarker

> ftrace_marker.cpp:85
> +{
> +    (*s_writeFunction)(m_file, message + QStringLiteral(" (begin_ctx=%1)").arg(ctx));
> +}

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

> ftrace_marker.cpp:104
> +        const int end = line.indexOf(' ', start);
> +        return QFileInfo(QDir(line.mid(start, end - start)),
> +                         QString::fromLatin1("trace_marker"));

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

> ftrace_marker.h:42-43
> +    void print(const QString &message);
> +    void print(ulong ctx, const QString &message);
> +    void print(const QString &message, ulong ctx);
> +

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
> +        <arg type="b" direction="in"/>
> +        <arg type="b" direction="out"/>
> +    </method>

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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D23114

To: romangg, #kwin
Cc: davidedmundson, apol, zzag, kwin, LeGast00n, The-Feren-OS-Dev, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20190828/411d05bc/attachment-0001.html>