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

[Carl Worth]

José Fonseca <jose.r.fonseca at gmail.com> writes:
> Merged,

Thanks for all the review and merging, José! I really appreciate it.

I've updated my branch based on your merges.

I'll next update things based on your comments below, and push the
trim-related changes to the master branch as you've suggested.

> 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.

That sounds great. I'll continue to post descriptions of changes to the
list, and code itself when I want review. (If anyone wants to see more
code than I'm posting, please let me know. I'm always happy to post code
if people want to read it, but I don't want to fill the list with
patches if people aren't reading them.)

>   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

OK. I'll fix these on my side.

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

I don't actually know much about these specific calls. I was just on the
lookout for calls that don't affect GL state directly, but where the
user would really still want to keep the call in the trace.

glStringMarkerGREMEDY is the most obvious call in this class, but all
the calls with "Debug" in their name seemed likely to me as well. I may
have captured more than intended here. Let me know if you feel these are
really important to trim.

> "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.

OK. Do take a look at it. As mentioned in the commit log, the basic
skip-list implementation has been well-tested within cairo, so I trust
it to be quite sound.

The extension of the skip-list to support a range at each node, (in
"Extend trim::CallSet with a new range-based add method.") is new here
and could definitely use some good review.

Eric did ask me in person why I chose a skip-list here rather than
something like a bitset. A bit-set could also be used, but I did not
think of it originally. For a call-set that's not sparse, (such as the
final result of calls needed after trimming with dependencies), a simple
bit-set could have both memory and performance advantages over the
skip-list, (though I haven't measured).

For a very sparse callset, (such as what you might have with something
on the command-line like "--calls=297653-"), a naive bit-set
implementation would be quite wasteful of memory. One could add various
optimizations for compressing large ranges of 1s and 0s in the bit-set,
but I can imagine the code getting more complex than the current
skip-list code for similar performance on sparse sets.

Another option would be to use something else in STL other than
std::set<unsigned> which I measured to perform quite badly on my system
at least. Perhaps vector<bool>?

I don't really care what the final implementation uses as long as it
performs well. The current linear search with std::list<CallRange> in
class CallSet is definitely inadequate for use cases I am hitting
regularly now.

> 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.

Thanks again.

> 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.

Sure, I'll do that. That will make it easier to know whether the
code I'm working on is mine to do with as I see fit, or if I should be
submitting things to you.

-Carl

-- 
carl.d.worth at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/apitrace/attachments/20130412/cfa830b7/attachment.pgp>