Add GetStubStats and related functions in GpuMemoryManager.

Provide stub implementations of GetStubStats and related functions in GpuMemoryManager.



BUG=131650
TEST=GpuMemoryManagerTest.GetStubStatsForLastManageTests content unittest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147475

[mmocny, 2012-07-18 15:08:13]

@ccameron and I are working towards adding pyauto(?) tests for GpuMemoryManager
(see http://codereview.chromium.org/10702200/).  This patch puts up stub
implementations for the proposed stats related functions.

This way, he can start writing tests in terms of the intended api.

[piman, 2012-07-18 17:11:25]

Some style nits. At a high level, the structures seem fine, but since the stats
are not filled, it doesn't really make a lot of sense to expose it. How about
adding the code that computes the stats in this CL?

https://chromiumcodereview.appspot.com/10801014/diff/4001/content/common/gpu/...
File content/common/gpu/gpu_memory_allocation.h (right):

https://chromiumcodereview.appspot.com/10801014/diff/4001/content/common/gpu/...
content/common/gpu/gpu_memory_allocation.h:103: {
nit: { on previous line

https://chromiumcodereview.appspot.com/10801014/diff/4001/content/common/gpu/...
content/common/gpu/gpu_memory_allocation.h:108: : min_allocation_bytes(0),
ideal_allocation_bytes(0)
nit: indent (+4 before :), style (ideal_allocation_bytes should go on next line,
'{' should be on same line as ideal_allocation_bytes)
(here and below)

https://chromiumcodereview.appspot.com/10801014/diff/4001/content/common/gpu/...
content/common/gpu/gpu_memory_allocation.h:118: return min_allocation_bytes ==
other.min_allocation_bytes &&
nit: indent (+2)

https://chromiumcodereview.appspot.com/10801014/diff/4001/content/common/gpu/...
content/common/gpu/gpu_memory_allocation.h:119: ideal_allocation_bytes ==
other.ideal_allocation_bytes;
nit: indent (+4 for continuations)

[mmocny, 2012-07-18 18:33:35]

Fixed nits, added simple implementation for stats given the current manage
function, and added unittest.

[piman, 2012-07-18 19:59:58]

LGTM + many nits. Please address before landing.

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
File content/common/gpu/gpu_memory_allocation.h (right):

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_allocation.h:112: :
min_allocation_bytes(min_bytes),
nit: extra 2 spaces indent.

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
File content/common/gpu/gpu_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_manager.cc:52: stub_memory_stats,
nit: Passing parameters by non-const references is frowned upon by the style
guide. You can simply replace by a pointer.
You should also make the next argument a const ref since you're not modifying
it.

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_manager.cc:233:
AssignMemoryAllocations(stub_memory_stats_,
nit: correct style is either

Function(param1,
         param2,
         param3);

or

Function(
    param1,
    param2,
    param3);

it was wrong before, but please fix it as you're going through here.

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_manager.cc:269: it = stub_memory_stats_.begin();
it != stub_memory_stats_.end(); ++it) {
nit: The typedef I suggest will help you here, but in general in this case it
would be better to write 
for (VeryLongType::iterator it =
        container.begin();
    it != container.end;
    ++it) {
}

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_manager.cc:275: 
nit: unnecessary empty line.

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
File content/common/gpu/gpu_memory_manager.h (right):

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_manager.h:74: std::map<GpuCommandBufferStubBase*,
StubMemoryStat>
I suggest returning const std::map<...>& to avoid copying this big data
structure.
Also, I suggest creating a typedef std::map<GpuCommandBufferStubBase*,
StubMemoryStat> StubMemoryStatMap.
Finally, you may be better served by a hash_map than a std::map, since you don't
care about the order of keys - it's cheaper in both memory and perf - ammortized
O(1) instead of O(log n).

https://chromiumcodereview.appspot.com/10801014/diff/7002/content/common/gpu/...
content/common/gpu/gpu_memory_manager.h:86: size_t
GetPeakAssignedAllocationSum() const {
nit: usually for trivial inline functions that just return a field, we name them
after the field itself. In this case, peak_assigned_allocation_sum().

[mmocny, 2012-07-18 20:50:02]

Thanks for review.

I had to add a hash function definition since it isn't implemented for pointers
by default.  Copied other examples from the codebase but they seem to just
reinterpret_cast, which I don't think is a good hash function and may produce
poor perf, right?  Should I use the existing integral value hash to hash the
reinterpreted pointer value instead?

Also, I renamed to stub_memory_stats_for_last_manage since it is also trivially
returning a member.

[piman, 2012-07-18 21:06:23]

A lot more went into this latest patch - is that intended?

About the hash function - sorry about that, I hadn't realized. It looks fine.
You'll probably need to override on MSVC as well (trybots will tell you).

[mmocny, 2012-07-18 21:09:27]

That wasn't intended, sorry.  I branched from a local branch and now it uploads
diffs from that by default (is there a way to reset branch HEAD to master, or
must you create a new branch and apply the diff?).

As far as the hash function, I haven't seen other examples in the codebase
adding msvc versions.. maybe there is a default hash function for pointers
there?  I'll run tryjob on windows to test.

On 2012/07/18 21:06:23, piman wrote:
> A lot more went into this latest patch - is that intended?
> 
> About the hash function - sorry about that, I hadn't realized. It looks fine.
> You'll probably need to override on MSVC as well (trybots will tell you).

[commit-bot: I haz the power, 2012-07-19 16:35:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10801014/11003

[commit-bot: I haz the power, 2012-07-19 17:46:01]

Change committed as 147475