std::bitset solution for GL Query cache.

std::bitset solution for GL Query cache.

This uses even less memory (a few hundred bytes) by sacrificing some
worst case performance (in the order of a microsecond per query) and
maintainability.

Estimated memory savings will be 9-12 KB. (3 KB per QuerySyncManager).

BUG=485536
R=reveman@chromium.org,jbauman@chromium.org

Committed: https://crrev.com/f06df73a8298d34510531fb489be6a189ee71a13
Cr-Commit-Position: refs/heads/master@{#330933}

[reveman, 2015-05-18 19:58:43]

https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/query_tracker.cc (right):

https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/query_tracker.cc:75: // O(n^2) for the common
scenario.
is this really worthwhile? i would prefer if we kept this super simply and just
scanned through the bits from the start each time.

https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/query_tracker.h (right):

https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/query_tracker.h:40: uint32 in_use_query_count;
Can we use in_use_queries.count() instead?

[Daniel Bratell, 2015-05-19 07:37:26]

On 2015/05/18 19:58:43, reveman wrote:
>
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/query_tracker.cc (right):
> 
>
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> gpu/command_buffer/client/query_tracker.cc:75: // O(n^2) for the common
> scenario.
> is this really worthwhile? i would prefer if we kept this super simply and
just
> scanned through the bits from the start each time.
> 
>
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/query_tracker.h (right):
> 
>
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> gpu/command_buffer/client/query_tracker.h:40: uint32 in_use_query_count;
> Can we use in_use_queries.count() instead?

These two optimizations are to keep the performance of this similar to the old
code. Without both of them CreateQuery() it will need three times as much time
as the old code (i.e. about 2 microseconds extra per CreateQuery on a fast
workstation).

If we just focus on size and classify a few microseconds per CreateQuery() as
noise then removing these are trivial. Not being deeply familiar with this area
of Chromium and its performance characteristics I will leave that judgment to
you.

[reveman, 2015-05-19 18:13:30]

On 2015/05/19 at 07:37:26, bratell wrote:
> On 2015/05/18 19:58:43, reveman wrote:
> >
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> > File gpu/command_buffer/client/query_tracker.cc (right):
> > 
> >
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> > gpu/command_buffer/client/query_tracker.cc:75: // O(n^2) for the common
> > scenario.
> > is this really worthwhile? i would prefer if we kept this super simply and
just
> > scanned through the bits from the start each time.
> > 
> >
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> > File gpu/command_buffer/client/query_tracker.h (right):
> > 
> >
https://codereview.chromium.org/1129253006/diff/20001/gpu/command_buffer/clie...
> > gpu/command_buffer/client/query_tracker.h:40: uint32 in_use_query_count;
> > Can we use in_use_queries.count() instead?
> 
> These two optimizations are to keep the performance of this similar to the old
code. Without both of them CreateQuery() it will need three times as much time
as the old code (i.e. about 2 microseconds extra per CreateQuery on a fast
workstation).

I don't think we need to worry much about the performance here but why don't you
reduce kSyncsPerBucket to 256 as part of this patch? Sounds like that would not
only reduce memory usage significantly but also improve performance. 1024 seems
like a lot based on the sync query usage I'm aware of.

[Daniel Bratell, 2015-05-20 14:18:10]

On 2015/05/19 18:13:30, reveman wrote:
> I don't think we need to worry much about the performance here but why don't
you
> reduce kSyncsPerBucket to 256 as part of this patch? Sounds like that would
not
> only reduce memory usage significantly but also improve performance. 1024
seems
> like a lot based on the sync query usage I'm aware of.

I've removed the optimizations which hurts, but making it smaller recovers 
some of the time.

Also, the small performance hit is in the unlikely (as far as we know) scenario 
where there is a lot of queries in flight at the same time. For the trivial case
the code will find a free query in the first slot and there will be no looping.

I did increase the number of mock buffers since the smaller buckets mean that
there will be more of them allocated.

[reveman, 2015-05-20 14:35:14]

lgtm with nits

https://codereview.chromium.org/1129253006/diff/40001/gpu/command_buffer/clie...
File gpu/command_buffer/client/query_tracker.cc (right):

https://codereview.chromium.org/1129253006/diff/40001/gpu/command_buffer/clie...
gpu/command_buffer/client/query_tracker.cc:88:
DCHECK(info.bucket->in_use_queries.count() > 0);
nit: please use DCHECK_NE(info.bucket->in_use_queries.count(), 0u) here

https://codereview.chromium.org/1129253006/diff/40001/gpu/command_buffer/clie...
gpu/command_buffer/client/query_tracker.cc:90:
DCHECK(info.bucket->in_use_queries[index_in_bucket] == true);
nit: DCHECK(info.bucket->in_use_queries[index_in_bucket])

[Daniel Bratell, 2015-05-20 15:00:20]

Done!

jbauman, I think we need your final approval still?

[jbauman, 2015-05-20 22:50:12]

lgtm

[Daniel Bratell, 2015-05-21 09:33:46]

The CQ bit was checked by bratell@opera.com

[Daniel Bratell, 2015-05-21 09:33:46]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1129253006/#ps60001
(title: "Different DCHECKs")

[commit-bot: I haz the power, 2015-05-21 09:34:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129253006/60001

[commit-bot: I haz the power, 2015-05-21 12:08:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-21 12:09:34]

Patchset 4 (id:??) landed as
https://crrev.com/f06df73a8298d34510531fb489be6a189ee71a13
Cr-Commit-Position: refs/heads/master@{#330933}