Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1038783002:
Add switch (for cast_shell) to defer creation of GPU process. (Closed)
DescriptionAdd switch (for cast_shell) to defer creation of GPU process.
For context - on Chromecast, we have the following scenario:
* We support running in the background (i.e. not rendering anything)
with our service running in the browser process to listen for incoming
cast connections.
* We cannot acquire any GPU resources (e.g. a GL context or surface)
during this time as another external application needs it, and it's a
global resource.
We have supported starting up in the foreground and switching to this
mode for a long time.
But now, we need to support starting up in this mode. In other words,
we need to bring up the browser process and run our cast receiver
service, but we cannot run any of the GPU process's initialisation
code until an incoming cast arrives.
BUG=internal b/19898960
Committed: https://crrev.com/db7efe6dfbda08faa1e55cda4441b43dac005733
Cr-Commit-Position: refs/heads/master@{#322655}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed CONTENT_EXPORT #
Total comments: 2
Patch Set 3 : Comment+naming update, reenable tests #
Total comments: 3
Patch Set 4 : Ensure BrowserGpuChannelHostFactory::Initialize is called. #
Total comments: 2
Patch Set 5 : A nit #Patch Set 6 : Add CONTENT_EXPORT to switch #Patch Set 7 : Disabled test on Chromeos, it won't pass there #Patch Set 8 : Tests appear to be flaky on Windows, disabling there. #
Messages
Total messages: 42 (15 generated)
halliwell@chromium.org changed reviewers: + nasko@chromium.org
nasko@chromium.org changed reviewers: + enne@chromium.org
Codewise this looks fine (modulo one comment). Including enne@ to see if this makes sense from GPU architecture perspective. https://codereview.chromium.org/1038783002/diff/1/content/public/common/conte... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1038783002/diff/1/content/public/common/conte... content/public/common/content_switches.h:32: CONTENT_EXPORT extern const char kDeferGpuProcessCreation[]; Why does this need CONTENT_EXPORT?
https://codereview.chromium.org/1038783002/diff/1/content/public/common/conte... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1038783002/diff/1/content/public/common/conte... content/public/common/content_switches.h:32: CONTENT_EXPORT extern const char kDeferGpuProcessCreation[]; On 2015/03/26 18:17:44, nasko wrote: > Why does this need CONTENT_EXPORT? Actually I was just not familiar with what the flag means and was following the previous line :) Are these for the component build? We're not using that so I can just remove.
enne@chromium.org changed reviewers: + piman@chromium.org
+piman,jbauman,danakj for opinions on ^
danakj@chromium.org changed reviewers: + danakj@chromium.org, sievers@chromium.org
+sievers Naively this seems okay, though I wish it wasn't a command line flag (like most command line flags that will be forever)
lgtm https://codereview.chromium.org/1038783002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1038783002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:69: // for creating GPU process before it's needed. nit: I don't think the last sentence is correct. We'll still go ahead and create the GPU channel if we need one (see GpuProcessTransportFactory::CreateOutputSurface), this flag just disables the prelaunch of the GPU process.
On 2015/03/26 20:26:19, piman (Very slow to review) wrote: > lgtm > > https://codereview.chromium.org/1038783002/diff/20001/content/public/common/c... > File content/public/common/content_switches.cc (right): > > https://codereview.chromium.org/1038783002/diff/20001/content/public/common/c... > content/public/common/content_switches.cc:69: // for creating GPU process before > it's needed. > nit: I don't think the last sentence is correct. We'll still go ahead and create > the GPU channel if we need one (see > GpuProcessTransportFactory::CreateOutputSurface), this flag just disables the > prelaunch of the GPU process. Yes, probably makes sense though, since wanting to create an output surface should imply that you have a window now and can create gpu resources. So maybe there is a better name for the switch, since it really just disables an optimization, which is to proactively spin up the channel to speed up startup time (and avoid blocking later). I fail to come up with a good name.. kDisableEarlyGPUInit? :)
btw, since there will be no testing and also no reference to the switch you are
adding upstream:
If you feel like it, you can probably make some very small changes in
gpu_ipc_browsertests.cc to take advantage of this:
See '// Start all tests without a gpu channel so that the tests exercise a
// consistent codepath.'.. change the if-condition to DCHECK() and override
SetUpCommandLine(base::CommandLine* command_line) to add your switch
On 2015/03/26 20:40:35, sievers wrote: > btw, since there will be no testing and also no reference to the switch you are > adding upstream: > > If you feel like it, you can probably make some very small changes in > gpu_ipc_browsertests.cc to take advantage of this: > > See '// Start all tests without a gpu channel so that the tests exercise a > // consistent codepath.'.. change the if-condition to DCHECK() and override > SetUpCommandLine(base::CommandLine* command_line) to add your switch It looks like those tests were all disabled because they all assert that the gpu channel isn't established. I added this flag and re-enabled them ...
https://codereview.chromium.org/1038783002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1038783002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:69: // for creating GPU process before it's needed. On 2015/03/26 20:26:19, piman (Very slow to review) wrote: > nit: I don't think the last sentence is correct. We'll still go ahead and create > the GPU channel if we need one (see > GpuProcessTransportFactory::CreateOutputSurface), this flag just disables the > prelaunch of the GPU process. I see, yes. Removed this part of the comment.
https://codereview.chromium.org/1038783002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_ipc_browsertests.cc (right): https://codereview.chromium.org/1038783002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_ipc_browsertests.cc:84: CHECK(GetFactory()); Doesn't this fail? Don't you still have to call BrowserGpuChannelHostFactory::Initialize(false)?
https://codereview.chromium.org/1038783002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_ipc_browsertests.cc (right): https://codereview.chromium.org/1038783002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_ipc_browsertests.cc:84: CHECK(GetFactory()); On 2015/03/26 22:05:29, sievers wrote: > Doesn't this fail? Don't you still have to call > BrowserGpuChannelHostFactory::Initialize(false)? Good point. The test was passing for me, but that may be platform-dependent? (BrowserGpuChannelHostFactory::Initialize was still called for me, from BrowserMainLoop::BrowserThreadsStarted, but there are various platform ifdefs).
lgtm https://codereview.chromium.org/1038783002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_ipc_browsertests.cc (right): https://codereview.chromium.org/1038783002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_ipc_browsertests.cc:84: CHECK(GetFactory()); On 2015/03/26 22:16:52, halliwell wrote: > On 2015/03/26 22:05:29, sievers wrote: > > Doesn't this fail? Don't you still have to call > > BrowserGpuChannelHostFactory::Initialize(false)? > > Good point. The test was passing for me, but that may be platform-dependent? > (BrowserGpuChannelHostFactory::Initialize was still called for me, from > BrowserMainLoop::BrowserThreadsStarted, but there are various platform ifdefs). AURA || MAC || ANDROID might cover everything. But feel free to go with what the bots say :) I think either way this is fine because each test calls DCHECK(!IsChannelEstablished()) which makes sure that your cmdline works. https://codereview.chromium.org/1038783002/diff/60001/content/browser/gpu/gpu... File content/browser/gpu/gpu_ipc_browsertests.cc (right): https://codereview.chromium.org/1038783002/diff/60001/content/browser/gpu/gpu... content/browser/gpu/gpu_ipc_browsertests.cc:97: command_line->AppendSwitchASCII(switches::kDisableGpuEarlyInit, ""); nit: AppendSwitch(switches::kDisableGpuEarlyInit)
https://codereview.chromium.org/1038783002/diff/60001/content/browser/gpu/gpu... File content/browser/gpu/gpu_ipc_browsertests.cc (right): https://codereview.chromium.org/1038783002/diff/60001/content/browser/gpu/gpu... content/browser/gpu/gpu_ipc_browsertests.cc:97: command_line->AppendSwitchASCII(switches::kDisableGpuEarlyInit, ""); On 2015/03/26 23:16:19, sievers wrote: > nit: AppendSwitch(switches::kDisableGpuEarlyInit) Done.
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1038783002/#ps80001 (title: "A nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038783002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2015/03/26 23:46:39, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) I presume this means I _do_ need CONTENT_EXPORT on the switch after all ... ? :)
On 2015/03/26 23:49:47, halliwell wrote: > On 2015/03/26 23:46:39, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > > I presume this means I _do_ need CONTENT_EXPORT on the switch after all ... ? :) You do now, since you added it to browsertests, which is a separate executable. It is fine to add, but your initial version didn't actually use it anywhere outside of the binary.
On 2015/03/26 23:52:12, nasko wrote: > On 2015/03/26 23:49:47, halliwell wrote: > > On 2015/03/26 23:46:39, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > > > > I presume this means I _do_ need CONTENT_EXPORT on the switch after all ... ? > :) > > You do now, since you added it to browsertests, which is a separate executable. > It is fine to add, but your initial version didn't actually use it anywhere > outside of the binary. Yep ok, makes sense - thanks :)
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1038783002/#ps100001 (title: "Add CONTENT_EXPORT to switch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038783002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1038783002/#ps120001 (title: "Disabled test on Chromeos, it won't pass there")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038783002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1038783002/#ps140001 (title: "Tests appear to be flaky on Windows, disabling there.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1038783002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/db7efe6dfbda08faa1e55cda4441b43dac005733 Cr-Commit-Position: refs/heads/master@{#322655}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1043603002/ by danakj@chromium.org. The reason for reverting is: Breaking mac browser tests? http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/436 BrowserGpuChannelHostFactoryTest.AlreadyEstablished (run #1): [ RUN ] BrowserGpuChannelHostFactoryTest.AlreadyEstablished [35311:27139:0327/154011:20111614311426:ERROR:browser_gpu_channel_host_factory.cc(134)] Failed to launch GPU process. [35311:27139:0327/154011:20111614504937:ERROR:browser_gpu_channel_host_factory.cc(134)] Failed to launch GPU process. ../../content/browser/gpu/gpu_ipc_browsertests.cc:179: Failure Value of: event Actual: false Expected: true [ FAILED ] BrowserGpuChannelHostFactoryTest.AlreadyEstablished, where TypeParam = and GetParam() = (215 ms) . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
