Incorporating FrameRetrace upstream into ApiTrace

Thu May 2 04:52:32 UTC 2019

José Fonseca <jfonseca at vmware.com> writes:

> Hi Mark,
>
> Sorry for the delay.  You caught me when I was on vacation, and I've been
> busy catching up with things after coming back.
>
> I haven't had time to play with FrameRetrace much.
>
> First of all I want to reiterate some of my initials comments about
> FrameRetrace a while back, when it was first announced, on
> https://lists.freedesktop.org/archives/apitrace/2015-June/001087.html .
>
> On one hand, it doesn't seem that FrameRetrace requires many changes to
> existing code, per the output of `git diff --diff-filter=DM $(git
> merge-base master frameretrace)..frameretrace` (seen
> https://pastebin.com/E5WNNG8U ), which is good.   Still, this needs to be
> cleaned, as a lot of these changes look spurious, and they should be
> committed to apitrace master ahead of an eventual frameretrace merge.
>
> But on the other hand, I still feel that several things were done
> differently (and unnecessarily redone in cases.)  Most noticeably: a
> completely separate GUI, with a completely different serialization format,
> using a separate daemon instead of building upon the existing glretrace
> slave processes.

The principal reason for the separation was to limit changes to
apitrace.  I figured it would be more controversial and harder to rebase
if I made structural changes to the existing GUI.  I also didn't see a
way to implement the workflow within the structure of qapitrace.

The serialization format is an arbitrary choice, and I could re-write
that layer to use JSON.  The data structures in the interface are deep,
and I just chose protocol buffers because it was typesafe and I have
already written too many serialization implementations for one career.
I can re-write using any format if it's a problem.

I need an asynchronous mechanism, because some of the callbacks carry a
lot of data (eg, dumping bound textures).  I had to implement a second
cancellation socket to get the latency below 30s.  Not sure how that
could work with a JSON pipe, but there must be a way.

> Even if I can accept a completely separate GUI on apitrace source tree (not
> ideal, because it will confuse users, and future contributors, but I might
> come to make peace with it), I can't accept the server as it stands.  In
> particular `daemon/glretrace_globals.cpp` needs to disappear somehow.  The
> server needs to look more like the existing glretrace/d3dretrace/etc
> (possibly it could even be merged into these), and not duplicate internal
> code.

Yes, the globals are totally a hack.  I couldn't figure out what the
right answer was.

> But honestly I feel it's too little too late.  Below you're focusing on use
> cases, whereas my concerns are all but that -- I care about the
> architecture and ease of maintenance. I don't doubt FrameRetrace is great
> for users.  But if the goal is to merge, then the right architecture should
> be chosen -- less duplication and more integration.
> 
> Plus fact you think you done all you planed to do is a big red flag to my
> ears. I'm already treading water with my apitrace maintenance.  This will
> be a butt load of code which I have little interest for.

What about a public retrace lib, that different UI's could link against?
I agree that FrameRetrace is a lot of code, and I'm not looking to dump
it on anyone.  I'm happy to maintain it as something other than a fork.

I'm not saying "I'm done with this thing, please take it off my hands".
More like, "This thing is done, people will be happy with it -- and I'd
like to stop rebasing."

> It's all GL specific - a dying API (even if a slow death.)

That's really the reason why I'm moving my focus to other work.  Intel's
Mesa team needs Vulkan performance tools.  If apitrace adds support for
Vulkan, then I'll be working on it again.

> A completely unknown code base.  The core ideas are indeed
> interesting, but they could be more easily achieved within the current
> components, as opposed to an wholesale re-implementation.
>
> Sorry, but I don't want to give false hopes: merging seems unlikely at this
> stage...

I appreciate the direct feedback.

> Jose
>
> PS: It's also surprising to see completely different indentation and code
> style. I know that's easily fixable nowadays with tools, but to me is a
> clear sign of how merging this code was a non-goal.  If the plan was to
> merge, why not just get these things right off the bat?

Doh, I haven't given a thought to that since I added a dir-locals file.
I'm sure that was immediately obvious to you, and an expensive mistake
on my part.  I will do the work to fix all of that.

> On Tue, Apr 16, 2019 at 8:16 PM Mark Janes <mark.a.janes at intel.com> wrote:
>
>> Hello José,
>>
>> I'm interested in getting the FrameRetrace UI upstream in the ApiTrace
>> project.  The benefits of doing this would be:
>>
>>  - When ApiTrace changes the trace format, I have to rebase the
>>    FrameRetrace branch.  This is a tedious and time consuming project.
>>
>>  - Upstream ApiTrace users would benefit from a far more responsive
>>    and fully-featured application for investigating rendering and
>>    performance problems.
>>
>>  - FrameRetrace would likely benefit from more users/collaborators.
>>
>> A possible *disadvantage* of upstreaming FrameRetrace would be
>> increased overhead due to ApiTrace merge/review process.  Is there a
>> way to incorporate FrameRetrace that wouldn't create additional work
>> for you?
>>
>> I have implemented nearly all of the features that I anticipated for
>> FrameRetrace.  The application is quite usable, and I have gotten
>> feedback from Mesa devs and OpenGL developers that they have quickly
>> investigated and solved problems with the tool.  OpenGL debugging
>> features work without metrics, but Mesa developers have enabled
>> hardware metrics for at least the following platforms:
>>
>>  - Intel (i965)
>>  - AMD (Radeon)
>>  - Broadcom (VC4)
>>  - Qualcomm (Freedreno)
>>  - Nvidia (Nouveau)
>>
>>    (caveat: I haven't done testing myself, but I fielded questions
>>    from devs that enabled the platforms and heard that they got
>>    metrics working).
>>
>> I have a bucket list of features to add or improve, but I am reluctant
>> to spend time on them without feedback from users to indicate the
>> value of the functionality.  I would eagerly invest time to make
>> FrameRetrace more suitable for upstream.  Examples might include:
>>
>>  - Documentation/screenshots/demos
>>  - Feedback from your review
>>  - CMake changes in ApiTrace that I made to enable FrameRetrace (it's
>>    probably not perfect)
>>  - Visibility changes that I made to expose ApiTrace data structures
>>  - Any bugs that end-users encounter with specific application traces.
>>
>> What is your opinion of FrameRetrace, and the possibility of
>> incorporating it into your project?  Are there more features that you
>> would like to see?  What information would help you evaluate the
>> proposal?
>>
>> Thanks,
>>
>> -Mark
>> _______________________________________________
>> apitrace mailing list
>> apitrace at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/apitrace