Add calls to inform gpu of browser window count

Add calls to inform gpu of browser window count


BUG=146448
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=160144

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=160174

[ccameron, 2012-10-02 02:06:43]

What happens when the GPU process is restarted?  We'll need to push the initial
# of windows over at GPU process restart.

Should we conditionalize some of this on OS_CHROMEOS? If so, it's probably the
memory manager part that should be conditionalized.

https://codereview.chromium.org/11026015/diff/1/chrome/browser/chrome_gpu_uti...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/11026015/diff/1/chrome/browser/chrome_gpu_uti...
chrome/browser/chrome_gpu_util.cc:191: void InitializeBrowserMonitor() {
Will leak detection complain about this?  If there a way we should scope this or
track it to make sure that we don't create multiple instances?

[DaveMoore, 2012-10-03 20:06:55]

I don't think this needs to be chromeos specific as other platforms will have
the same problems. It should never kick off a gpu process on its own and I've
now handled gpu process restarts.

https://codereview.chromium.org/11026015/diff/1/chrome/browser/chrome_gpu_uti...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/11026015/diff/1/chrome/browser/chrome_gpu_uti...
chrome/browser/chrome_gpu_util.cc:191: void InitializeBrowserMonitor() {
Changed it to a singleton that won't trigger leak detection.

On 2012/10/02 02:06:43, ccameron1 wrote:
> Will leak detection complain about this?  If there a way we should scope this
or
> track it to make sure that we don't create multiple instances?

[piman, 2012-10-03 20:12:05]

Note: do we have a bug filed to clean this up correctly? Worth mentioning
somewhere.

https://codereview.chromium.org/11026015/diff/16002/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11026015/diff/16002/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:452: return Send(new
GpuMsg_SetVideoMemoryWindowCount(0));
You most likely want the correct window count here.
Consider the case where you have several windows open and you navigate to a page
that is the first one to turn on compositing. You'll want accurate data at that
point. Also after a GPU process crash.

[ccameron, 2012-10-03 20:16:21]

https://codereview.chromium.org/11026015/diff/16002/chrome/browser/chrome_gpu...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/11026015/diff/16002/chrome/browser/chrome_gpu...
chrome/browser/chrome_gpu_util.cc:47: if (!installed_) {
(!installed_) should be (installed_)

[DaveMoore, 2012-10-03 20:25:00]

https://codereview.chromium.org/11026015/diff/16002/chrome/browser/chrome_gpu...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/11026015/diff/16002/chrome/browser/chrome_gpu...
chrome/browser/chrome_gpu_util.cc:47: if (!installed_) {
On 2012/10/03 20:16:22, ccameron1 wrote:
> (!installed_) should be (installed_)

Done.

[ccameron, 2012-10-03 20:29:59]

Sorry, pressed send too soon.  To get the right count at init, can we just call
back into gpu_util in gpu_process_host, or is that sort of thing not allowed?

https://codereview.chromium.org/11026015/diff/21002/chrome/browser/chrome_gpu...
File chrome/browser/chrome_gpu_util.h (right):

https://codereview.chromium.org/11026015/diff/21002/chrome/browser/chrome_gpu...
chrome/browser/chrome_gpu_util.h:19: void UninstallBrowserMonitor();
Could we add a
int GetBrowserMonitorWindowCount()
function here, and call it in GpuProcessHost::Init()?

https://codereview.chromium.org/11026015/diff/21002/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11026015/diff/21002/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:452: return Send(new
GpuMsg_SetVideoMemoryWindowCount(0));
Could we call gpu_util::GetBrowserMonitorWindowCount() here to get the right
count?

[piman, 2012-10-03 20:41:56]

On Wed, Oct 3, 2012 at 1:29 PM, <ccameron@chromium.org> wrote:

> Sorry, pressed send too soon.  To get the right count at init, can we just
> call
> back into gpu_util in gpu_process_host, or is that sort of thing not
> allowed?
>

It's not right (content can't call into chrome, let alone thread concerns)
but it can call into the GpuDataManagerImpl which has reasonably correct
data (modulo race).

Antoine


>
>
> https://codereview.chromium.**org/11026015/diff/21002/**
>
chrome/browser/chrome_gpu_**util.h<https://codereview.chromium.org/11026015/diff/21002/chrome/browser/chrome_gpu_util.h>
> File chrome/browser/chrome_gpu_**util.h (right):
>
> https://codereview.chromium.**org/11026015/diff/21002/**
>
chrome/browser/chrome_gpu_**util.h#newcode19<https://codereview.chromium.org/11026015/diff/21002/chrome/browser/chrome_gpu_util.h#newcode19>
> chrome/browser/chrome_gpu_**util.h:19: void UninstallBrowserMonitor();
> Could we add a
> int GetBrowserMonitorWindowCount()
> function here, and call it in GpuProcessHost::Init()?
>
> https://codereview.chromium.**org/11026015/diff/21002/**
>
content/browser/gpu/gpu_**process_host.cc<https://codereview.chromium.org/11026015/diff/21002/content/browser/gpu/gpu_process_host.cc>
> File content/browser/gpu/gpu_**process_host.cc (right):
>
> https://codereview.chromium.**org/11026015/diff/21002/**
>
content/browser/gpu/gpu_**process_host.cc#newcode452<https://codereview.chromium.org/11026015/diff/21002/content/browser/gpu/gpu_process_host.cc#newcode452>
>
> content/browser/gpu/gpu_**process_host.cc:452: return Send(new
> GpuMsg_**SetVideoMemoryWindowCount(0));
> Could we call gpu_util::**GetBrowserMonitorWindowCount() here to get the
> right count?
>
>
https://codereview.chromium.**org/11026015/<https://codereview.chromium.org/1...
>

[DaveMoore, 2012-10-03 21:26:54]

ccameron1 could talk better to the plan to redo the gpu allocation approach.

https://codereview.chromium.org/11026015/diff/16002/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11026015/diff/16002/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:452: return Send(new
GpuMsg_SetVideoMemoryWindowCount(0));
Bork.
Done.
On 2012/10/03 20:12:05, piman wrote:
> You most likely want the correct window count here.
> Consider the case where you have several windows open and you navigate to a
page
> that is the first one to turn on compositing. You'll want accurate data at
that
> point. Also after a GPU process crash.

[ccameron, 2012-10-03 22:05:42]

https://codereview.chromium.org/11026015/diff/23004/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11026015/diff/23004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:453:
GpuDataManagerImpl::GetInstance()->GpuAccessAllowed()));
s/GpuAccessAllowed/GetWindowCount/, I think.

[piman, 2012-10-03 22:10:21]

https://codereview.chromium.org/11026015/diff/23004/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11026015/diff/23004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:453:
GpuDataManagerImpl::GetInstance()->GpuAccessAllowed()));
On 2012/10/03 22:05:43, ccameron1 wrote:
> s/GpuAccessAllowed/GetWindowCount/, I think.

Yes...
If you get a chance, please double-check that you have meaningful data there in
case of GPU process crash recovery, e.g. by killing it.

[DaveMoore, 2012-10-03 22:52:38]

https://codereview.chromium.org/11026015/diff/23004/content/browser/gpu/gpu_p...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/11026015/diff/23004/content/browser/gpu/gpu_p...
content/browser/gpu/gpu_process_host.cc:453:
GpuDataManagerImpl::GetInstance()->GpuAccessAllowed()));
Bork Bork. I rushed and get messed up autocomplete.

I've put the right call in and in a test program added a printf() in the
GPUMemoryManager and confirmed it received the right number of windows after I
killed the gpu process.

On 2012/10/03 22:05:43, ccameron1 wrote:
> s/GpuAccessAllowed/GetWindowCount/, I think.

[piman, 2012-10-03 22:53:47]

lgtm

[ccameron, 2012-10-03 22:56:04]

lgtm

[DaveMoore, 2012-10-03 23:56:57]

Ben: OWNER please

[Ben Goodger (Google), 2012-10-04 16:01:22]

lgtm