Several new (or old) feature-branches proposed for upstream merge

[José Fonseca]

Merged,

On Sat, Mar 23, 2013 at 9:41 AM, Carl Worth <cworth at cworth.org> wrote:
> Hi José (and apitrace users),
>
> I've just pushed a new "cworth" branch to the apitrace repository which
> is an octopus merge of 6 branches that I am proposing for merging
> upstream.
>
> Some of these branches have been seen before (some of which have been
> modified or updated with new commits), and some are new. All have been
> rebased onto the most recent master branch and tested against the
> apitrace-tests repository.
>
> Here's a run-down of the changes in the branches. I think everything
> here should be entirely uncontroversial, but for one commit which I'll
> call out in more detail below:
>
>   remote-target
>
>         This is a feature to allow qapitrace to perform its replays on a
>         remote target via ssh. This commit has been updated according to
>         feedback from José (to avoid X11 forwarding over ssh so that
>         the remote X server is tested). The patch has also been tested
>         by the original user who requested the change and the
>         documentation has been updated significantly in response to that
>         testing.
>
>         This feature still has some (documented) limitations, such as
>         the trace file needing to be visible on the remote
>         system. José's previous suggestion to stream the trace over
>         stdin is still a viable future improvement. But the CLI will not
>         need to change for this, and the patch is useful already for the
>         user originally requesting this feature, so I believe it's ready
>         to be pushed as-is.

Merged.

>   trim-more-no-side-effects
>
>         This is a bug-fix series in response to a user who reported that
>         calls such as glGetInteger* were not being trimmed by:
>
>                 apitrace trim --auto --trim-spec=no-side-effects
>
>         In fact, the list of calls flagged as NO_SIDE_EFFECTS was
>         dramatically smaller than it should have been, and this series
>         expands the list significantly, (and to match the state of calls
>         with siedeefects=False in specs/glapi.py file).
>
>         There are also three related bug fixes in this series.

I don't plan to merge any trim specific commits. It's your baby so I
feel you should be empowred to own and maintain it yourself. Same goes
for "apitrace trim" default behaviour and command line parsing. I
still want to review and commit changes on common code though. That is
the focus on my comments below.

-- "trim: Greatly expand the list of calls considered to have no side effects"

  This commit has calls that have side effects:
  - glGetTextureImageEXT
 - glGetnCompressedTexImageARB
 - glGetnColorTableARB

I believe these have side effects when PBOs are on. But specs/glapi.py
also have the same error. Both need to be fixed. I'll go ahead and fix
glapi.py

--  "trim: Don't trim user-inserted debugging calls as having no side effect"

Please squash this with "trim: Greatly expand the list of calls
considered to have no side effects"

Also, "glGetDebugMessageLog" and "glDebugMessageLog" are not really
inserted by user, or are they? If not, they really should be marked as
no side effects.

>
>   cworth-bug-fixes
>
>         This series collects two miscellaneous bug fixes.
>
>         One is a bug where passing both --calls and --frames options to
>         trim would cause the trim code to bailout as soon as one list
>         was exhausted without continuing to process the other list. I've
>         added a new test case for this bug in the cworth-bug-fixes
>         branch pushed to apitrace-tests.

Feel free to go ahead and commit yourself.

>         The second bug is a missing glFlush in glretrace so that a user
>         can use "apitrace replay --wait" to see the results of a trace
>         even if the user happened to trim away the final glFlush or
>         glxSwapBuffers call from the original trace.

Commited.

>   trim-optimization
>
>         This series includes a really important optimization for
>         trimming which takes it from painfully-slow-as-to-be-unusable to
>         tolerably-slow on many workloads. I thought I had proposed the
>         first commit in this series earlier, but I don't recall getting
>         any feedback on it.

I don't remember seeing it. Must have missed.

>         The first commit in this series, (identical to what was submitted
>         previously), improves trim performance by changing internal
>         lists to use a much faster data structure (a small skip-list
>         implementation) instead of std::set<unsigned> which was used
>         before and was dominating the profiles.
>
>         The following three commits in the series are new as of this
>         submission. They change the code that accepts a call-list on the
>         "apitrace trim" command line to also use the skip-list (at least
>         when there is no step or frequency modifier). This resolves a
>         "TODO" comment in the code regarding switching to a binary tree
>         to speed up lookups.

"Use skiplist-based FastCallSet within trace::CallSet"

This is a complex change that changes trace::CallSet. I'll need more
time to review it. Feel free to commit the others.

>         For a few manually-entered ranges on the command-line, the
>         performance of these lookups is not at all significant. But I
>         ran into heinous performance problems when using a (very long)
>         call-list generated by "trim --print-callset" into a file and
>         then "trim --call=@<filename>". This series totally resolves the
>         performance bugs I hit.
>
>   replay-loop
>
>         This is a simple feature for "apitrace replay" which adds a
>         --loop option. This will have the visual appearance of --wait,
>         (in that the last frame remains visible), but the implementation
>         is totally different. What it does is continually loop over the
>         calls of the last frame, replaying them forever.
>
>         This is in response to a user who wanted exactly this behavior,
>         (so that while replaying the frame over and over, the user could
>         use other tools to investigate what is happening).
>
>         The replay does not currently do anything clever, such as
>         ensuring that the state is correctly restored to what it was
>         when originally at the beginning of the frame. Instead, it
>         naively replays the calls of the frame. For the few traces I've
>         thrown at this, (where, for example, I'm looping over one frame
>         in the middle of a game where things are already loaded), this
>         works just fine.
>
>         Obviously, we could improve this feature to be more robust in
>         the future. But I think it's already useful in many case.

Commited.

>   trim-auto-by-default
>
>         This is the only branch that I think might be controversial.
>
>         The first patch in the series adds the --no-deps, --no-prune,
>         and --exact options as I had originally provided when I first
>         wrote the trim code. These should be non-controversial. They are
>         well-defined, and allow for scripts to specify precisely the
>         behavior they want, (even if, for now, passing these options
>         doesn't change anything from the default).
>
>         The second patch effectively switches the default behavior of
>         "apitrace trim" from --exact to --auto. My rationale for this is
>         that only --auto can make sense as a default user interface.
>
>         Imagine a new user wanting to trim a trace down to a single
>         frame. A quick look at "apitrace trim --help" might lead to the
>         following reasonable command line:
>
>                 apitrace trim --frames=206
>
>         But with the default behavior of --exact, this command will
>         result in a non-working trace, (and this without even giving the
>         user any sort of warning that the trim won't be doing what's
>         expected).
>
>         I actually had a user encounter precisely this problem, and then
>         after encountering the broken resulting trace, the user tried to
>         debug with:
>
>                 apitrace trim --trim-spec=drawing --frames=206
>
>         But, of course, with --exact still effectively in place, this
>         command was behaving exactly the same and resulting in the same
>         broken trace.
>
>         This is particularly frustrating to me since
>         --trim-spec=drawing, (with --auto), is actually very robust. I
>         have not yet found a trace where it does not work correctly. (If
>         anyone does have one, pleas share it with me!)

It's up to you. For as along as you're actively maintaining and
triaging trim bugs, feel free to do what you think it's best with
trim.

It would be nice if all trim code lived in its own directory (e.g.,
cli/trim). In short, you'd be the maintainer of that subdir.  I'd only
commit if to fix Mac/Win/etc builds.

Jose