Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
65193003:
Clear SingleProcess and InProcessGPU logic. (Closed)
|
Created:
7 years, 1 month ago by dshwang Modified:
7 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, piman+watch_chromium.org Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionClear SingleProcess and InProcessGPU logic.
1. SingleProcess and InProcessGPU mode are not supported in multiple-dll mode
currently. Add CHECK() in RunNamedProcessTypeMain().
2. GPU Process.
1) Currently, if InProcessGPU mode is enabled and g_gpu_main_thread_factory is
not set, launch GPU Process slightly. However, other InProcessGPU code does not check if
g_gpu_main_thread_factory is not set. It is a potential bug.
2) GpuMain() can not be called when SingleProcess or InProcessGPU mode. Remove
SingleProcess and InProcessGPU switch check code.
3. Currently, if SingleProcess mode is enabled and g_renderer_main_thread_factory
is not set, launch Render Process slightly. It is a potential bug.
4. Currently, if SingleProcess mode is enabled and g_utility_main_thread_factory
is not set, launch Utility Process slightly. It is a potential bug.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237011
Patch Set 1 #
Total comments: 6
Patch Set 2 : WIP: diable kSingleProcess and kInProcessGPU in chrome multiple dll browser. #
Total comments: 1
Patch Set 3 : Add CommandLine::RemoveSwitch() to prevent --single-process when CHROME_MULTIPLE_DLL_BROWSER #
Total comments: 1
Patch Set 4 : Extract RegisterMainThreadFactories() from RunNamedProcessTypeMain() for readability. #
Total comments: 3
Patch Set 5 : --in-process-gpu is not supported in chrome multiple dll browser. #
Messages
Total messages: 31 (0 generated)
I'd like to ask piman and sievers to review this in place of me.
lgtm
On 2013/11/07 21:20:06, sievers wrote: > lgtm thank you for review! agl@ could you review content/app ? sky@ could you review content/browser ?
https://codereview.chromium.org/65193003/diff/1/content/app/content_main_runn... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/65193003/diff/1/content/app/content_main_runn... content/app/content_main_runner.cc:444: command_line.HasSwitch(switches::kInProcessGPU))); I suspect some people currently use that. What's broken exactly? https://codereview.chromium.org/65193003/diff/1/content/browser/gpu/gpu_proce... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/65193003/diff/1/content/browser/gpu/gpu_proce... content/browser/gpu/gpu_process_host.cc:587: CHECK(g_gpu_main_thread_factory); Should be DCHECK. https://codereview.chromium.org/65193003/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/65193003/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_process_host_impl.cc:475: CHECK(g_renderer_main_thread_factory); Should be DCHECK https://codereview.chromium.org/65193003/diff/1/content/browser/utility_proce... File content/browser/utility_process_host_impl.cc (right): https://codereview.chromium.org/65193003/diff/1/content/browser/utility_proce... content/browser/utility_process_host_impl.cc:152: CHECK(g_utility_main_thread_factory); Should be DCHECK.
Thanks for review. https://codereview.chromium.org/65193003/diff/1/content/app/content_main_runn... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/65193003/diff/1/content/app/content_main_runn... content/app/content_main_runner.cc:444: command_line.HasSwitch(switches::kInProcessGPU))); On 2013/11/07 22:09:52, piman wrote: > I suspect some people currently use that. What's broken exactly? under defined(CHROME_MULTIPLE_DLL_BROWSER), we don't register InProcessGPU thread constructor, CreateInProcessGpuThread, so we must not use --in-process-gpu. We launch GPU process if InProcessGPU thread constructor is not set although --in-process-gpu. if (in_process_ && g_gpu_main_thread_factory) { in_process_gpu_thread_->Start(); } else if (!LaunchGpuProcess(channel_id)) { return false; } However, we don't check g_gpu_main_thread_factory in other InProcessGPU code. e.g. gpu process termination code It can cause a bad bug. Utility and Render process has the same issue to gpu process. https://codereview.chromium.org/65193003/diff/1/content/browser/gpu/gpu_proce... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/65193003/diff/1/content/browser/gpu/gpu_proce... content/browser/gpu/gpu_process_host.cc:587: CHECK(g_gpu_main_thread_factory); On 2013/11/07 22:09:52, piman wrote: > Should be DCHECK. yes. but it will crash in next line. in_process_gpu_thread_->Start();
On Fri, Nov 8, 2013 at 2:05 AM, <dongseong.hwang@intel.com> wrote: > Thanks for review. > > > > https://codereview.chromium.org/65193003/diff/1/content/ > app/content_main_runner.cc > File content/app/content_main_runner.cc (right): > > https://codereview.chromium.org/65193003/diff/1/content/ > app/content_main_runner.cc#newcode444 > content/app/content_main_runner.cc:444: > command_line.HasSwitch(switches::kInProcessGPU))); > On 2013/11/07 22:09:52, piman wrote: > >> I suspect some people currently use that. What's broken exactly? >> > > under defined(CHROME_MULTIPLE_DLL_BROWSER), we don't register > InProcessGPU thread constructor, CreateInProcessGpuThread, so we must > not use --in-process-gpu. > Can we fix this instead? > We launch GPU process if InProcessGPU thread constructor is not set > although --in-process-gpu. > if (in_process_ && g_gpu_main_thread_factory) { > in_process_gpu_thread_->Start(); > } else if (!LaunchGpuProcess(channel_id)) { > return false; > } > > However, we don't check g_gpu_main_thread_factory in other InProcessGPU > code. e.g. gpu process termination code > It can cause a bad bug. > > Utility and Render process has the same issue to gpu process. > > > https://codereview.chromium.org/65193003/diff/1/content/ > browser/gpu/gpu_process_host.cc > File content/browser/gpu/gpu_process_host.cc (right): > > https://codereview.chromium.org/65193003/diff/1/content/ > browser/gpu/gpu_processcc_unittests_host.cc#newcode587<https://codereview.chromium.org/65193003/diff/1/content/browser/gpu/gpu_process_host.cc#newcode587> > content/browser/gpu/gpu_process_host.cc:587: > CHECK(g_gpu_main_thread_factory); > On 2013/11/07 22:09:52, piman wrote: > >> Should be DCHECK. >> > > yes. but it will crash in next line. > in_process_gpu_thread_->Start(); > If it's correct to have in_process_ && !g_gpu_main_thread_factory, then it needs to be gracefully handled. If it's incorrect to have in_process_ && !g_gpu_main_thread_factory, then all callers need to be audited and possibly fixed to make sure it doesn't happen, and then a DCHECK added for documentation and/or protect from future changes. Either way, there's no reason to add a check in production that would crash, as well as the strings for the message. > https://codereview.chromium.org/65193003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/11/08 21:48:19, piman wrote: Thank you for good opinion. Marshall Greenblatt reported --single-process is broken in Window in chromium-dev: https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... This CL can fix it. This CL is WIP. I need to test more my CommandLine change and add unittests. However, review is welcome :) Could I listen to your opinion whether this CL's approach is good or bad? browser/gpu/gpu_processcc_unittests_host.cc#newcode587<https://codereview.chromium.org/65193003/diff/1/content/browser/gpu/gpu_process_host.cc#newcode587> > > content/browser/gpu/gpu_process_host.cc:587: > > CHECK(g_gpu_main_thread_factory); > > On 2013/11/07 22:09:52, piman wrote: > > yes. but it will crash in next line. > > in_process_gpu_thread_->Start(); > > > > If it's correct to have in_process_ && !g_gpu_main_thread_factory, then it > needs to be gracefully handled. > If it's incorrect to have in_process_ && !g_gpu_main_thread_factory, then > all callers need to be audited and possibly fixed to make sure it doesn't > happen, and then a DCHECK added for documentation and/or protect from > future changes. > Either way, there's no reason to add a check in production that would > crash, as well as the strings for the message. I agree. However, adding check (process_ && !g_gpu_main_thread_factory) code in all place is very burden. So I add CommandLine::RemoveSwitch method, and remove kSingleProcess and kInProcessGPU switch if multiple dll browser.
https://codereview.chromium.org/65193003/diff/170001/content/shell/app/shell_... File content/shell/app/shell_main_delegate.cc (right): https://codereview.chromium.org/65193003/diff/170001/content/shell/app/shell_... content/shell/app/shell_main_delegate.cc:190: #endif How about this code? It looks good to me. :)
I add CommandLine::RemoveSwitch() to prevent --single-process when CHROME_MULTIPLE_DLL_BROWSER piman@, could you review again? I think this CL resolves all your concerns. agl@ could you review content/app ? sky@ could you review content/browser ?
thakis@, could you review command_line change?
Do we CHECK early on with a suitable warning in chrome some where?
On 2013/11/18 16:36:59, sky wrote: > Do we CHECK early on with a suitable warning in chrome some where? Thanks for review. https://codereview.chromium.org/65193003/diff/230001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/65193003/diff/230001/content/app/content_main... content/app/content_main_runner.cc:451: } I think here is suitable place to warn, because there is registering code above for thread mode. If you think it is not good, could you recommend better place?
Hopefully agl knows if this is the right place.
https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... content/app/content_main_runner.cc:422: "--single-process is not supported in chrome multiple dll browser."; Can't you just LOG(FATAL)?
https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... content/app/content_main_runner.cc:422: "--single-process is not supported in chrome multiple dll browser."; On 2013/11/19 00:35:16, Nico (in Tokyo until Nov 25) wrote: > Can't you just LOG(FATAL)? piman@ mentioned in https://codereview.chromium.org/65193003/#msg7 that we should not crash in our production. imho, running well with error msg is better than crash with fatal msg.
https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... content/app/content_main_runner.cc:422: "--single-process is not supported in chrome multiple dll browser."; On 2013/11/19 11:12:01, dshwang wrote: > On 2013/11/19 00:35:16, Nico (in Tokyo until Nov 25) wrote: > > Can't you just LOG(FATAL)? > > piman@ mentioned in https://codereview.chromium.org/65193003/#msg7 > that we should not crash in our production. > > imho, running well with error msg is better than crash with fatal msg. I'm ok with a LOG(FATAL) here, that explains what's going on and gives steps to resolve the problem.
On 2013/11/19 20:35:56, piman wrote: > https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... > File content/app/content_main_runner.cc (right): > > https://codereview.chromium.org/65193003/diff/330001/content/app/content_main... > content/app/content_main_runner.cc:422: "--single-process is not supported in > chrome multiple dll browser."; > On 2013/11/19 11:12:01, dshwang wrote: > > On 2013/11/19 00:35:16, Nico (in Tokyo until Nov 25) wrote: > > > Can't you just LOG(FATAL)? > > > > piman@ mentioned in https://codereview.chromium.org/65193003/#msg7 > > that we should not crash in our production. > > > > imho, running well with error msg is better than crash with fatal msg. > > I'm ok with a LOG(FATAL) here, that explains what's going on and gives steps to > resolve the problem. Ok, thank you for your opinion. I'll rollback command_line change, and use LOG(FATAL) here in the next patch set.
Nico@ piman@ I apply your advice to Patch Set 5. Alert LOG(FATAL) in chrome multiple dll browser when --single-process or --in-process-gpu. Could you review again? agl@ could you review content/app ? sky@ could you review content/browser ?
lgtm
base/ lgtm in patch set 5 ;-)
On 2013/11/20 01:16:05, Nico (in Tokyo until Nov 25) wrote: > base/ lgtm in patch set 5 ;-) yeah, I think base/ is fine too ;) agl@ could you review content/app ? sky@ could you review content/browser ?
> agl@ could you review content/app ? > sky@ could you review content/browser ? ping
piman is an OWNER here too. You don't need both of us. -sky
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/65193003/460001
On 2013/11/22 14:25:32, sky wrote: > piman is an OWNER here too. You don't need both of us. > > -sky aha, thank you.
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/65193003/460001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/65193003/460001
Message was sent while issue was closed.
Change committed as 237011 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
