Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
999883002:
Profiler-instrumentation of the startup time. (Closed)
DescriptionProfiler-instrumentation of the startup time.
Profiler recording is enabled right after command line is initialized. (We need command line to be initialized because we need get channel info first to determine whether or not to enable profiling). function is added inside ChromeMainDelegate and will be called from content.
BUG=453640
Committed: https://crrev.com/9e6a5ab378cec3edf0eeba1b6867db055d32d2a7
Cr-Commit-Position: refs/heads/master@{#321166}
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 8
Patch Set 3 : Address comments. #
Total comments: 12
Patch Set 4 : Address comments. #
Total comments: 2
Patch Set 5 : Address comments. #
Total comments: 6
Patch Set 6 : Address comments. #Patch Set 7 : Address comments #Patch Set 8 : Correct format error. #
Messages
Total messages: 35 (9 generated)
yiyaoliu@chromium.org changed reviewers: + vadimt@chromium.org
Hi vadim, PTAL. Tested on perf bots as well. Thanks, Yiyao
https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:976: void ChromeMainDelegate::EnableProfilerRecording() { Rename to MaybeEnableProfilerRecording. Or, make this function to return bool, and enable recording after at the caller's location. https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:978: case chrome::VersionInfo::CHANNEL_UNKNOWN: Shift right. (Are you using gil cl format, BTW?) https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:980: tracked_objects::ScopedTracker::Enable(); break after this line. https://codereview.chromium.org/999883002/diff/20001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/20001/content/browser/browser_... content/browser/browser_main_runner.cc:136: FROM_HERE_WITH_EXPLICIT_FUNCTION("BrowserMainRunnerImpl::Initialize")); "453640 BrowserMainRunnerImpl::Initialize" I know, this is how I originally wrote this, but this better highlights the temporary nature of this instrumentation.
https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:976: void ChromeMainDelegate::EnableProfilerRecording() { On 2015/03/12 17:54:56, vadimt wrote: > Rename to MaybeEnableProfilerRecording. > > Or, make this function to return bool, and enable recording after at the > caller's location. How about still enable it inside the function, but return a bool to indicate if it's enabled for real. https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:978: case chrome::VersionInfo::CHANNEL_UNKNOWN: On 2015/03/12 17:54:56, vadimt wrote: > Shift right. > (Are you using gil cl format, BTW?) Done. https://codereview.chromium.org/999883002/diff/20001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:980: tracked_objects::ScopedTracker::Enable(); On 2015/03/12 17:54:56, vadimt wrote: > break after this line. Done. https://codereview.chromium.org/999883002/diff/20001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/20001/content/browser/browser_... content/browser/browser_main_runner.cc:136: FROM_HERE_WITH_EXPLICIT_FUNCTION("BrowserMainRunnerImpl::Initialize")); On 2015/03/12 17:54:56, vadimt wrote: > "453640 BrowserMainRunnerImpl::Initialize" > I know, this is how I originally wrote this, but this better highlights the > temporary nature of this instrumentation. Done.
https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:980: tracked_objects::ScopedTracker::Enable(); We don't need BOTH returning bool and enabling. https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... content/app/content_main_runner.cc:572: delegate_->EnableProfilerRecording(); Please move this call to BrowserMainRunnerImpl::Initialize. https://codereview.chromium.org/999883002/diff/40001/content/public/app/conte... File content/public/app/content_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/40001/content/public/app/conte... content/public/app/content_main_delegate.h:80: virtual bool EnableProfilerRecording() {return false;} ShouldEnable... ?
https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:980: tracked_objects::ScopedTracker::Enable(); On 2015/03/12 18:23:03, vadimt wrote: > We don't need BOTH returning bool and enabling. I don't really have a strong opinion, but what's the benefit of enabling it from the caller instead of here? https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... content/app/content_main_runner.cc:572: delegate_->EnableProfilerRecording(); On 2015/03/12 18:23:03, vadimt wrote: > Please move this call to BrowserMainRunnerImpl::Initialize. This place runs earlier than that. we may want to intrument code that's earlier than BrowserMainRunnerImpl::Initialize? do we?
https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:980: tracked_objects::ScopedTracker::Enable(); Hypothetically, another implementation may override this, and there will be duplication of tracked_objects::ScopedTracker::Enable(); So, I have a slight preference for simply returning bool, but it's up to you. https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... content/app/content_main_runner.cc:572: delegate_->EnableProfilerRecording(); Unlikely. But we'll benefit from not spreading this logic across multiple places. Putting everything together will make it easier to understand and clean up code. (This is a strong opinion :) )
https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... content/app/content_main_runner.cc:572: delegate_->EnableProfilerRecording(); On 2015/03/12 18:37:26, vadimt wrote: > Unlikely. But we'll benefit from not spreading this logic across multiple > places. Putting everything together will make it easier to understand and clean > up code. > > (This is a strong opinion :) ) > I see. But this function takes in ContentMainParams, which has delegate as one of the params. However, the function you are suggesting takes in MainFunctionParams and it doesn't not have that delegate any more.
https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... content/app/content_main_runner.cc:572: delegate_->EnableProfilerRecording(); Then it's OK to leave it here. Please mark ALL added code with the comment to remove it after crbug.com/453640 is fixed.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/40001/chrome/app/chrome_main_d... chrome/app/chrome_main_delegate.cc:980: tracked_objects::ScopedTracker::Enable(); On 2015/03/12 18:37:26, vadimt wrote: > Hypothetically, another implementation may override this, and there will be > duplication of tracked_objects::ScopedTracker::Enable(); > > So, I have a slight preference for simply returning bool, but it's up to you. Changed as you suggested. https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/999883002/diff/40001/content/app/content_main... content/app/content_main_runner.cc:572: delegate_->EnableProfilerRecording(); On 2015/03/13 14:32:58, vadimt wrote: > Then it's OK to leave it here. Please mark ALL added code with the comment to > remove it after crbug.com/453640 is fixed. Done. https://codereview.chromium.org/999883002/diff/40001/content/public/app/conte... File content/public/app/content_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/40001/content/public/app/conte... content/public/app/content_main_delegate.h:80: virtual bool EnableProfilerRecording() {return false;} On 2015/03/12 18:23:03, vadimt wrote: > ShouldEnable... ? Done.
https://codereview.chromium.org/999883002/diff/80001/content/public/app/conte... File content/public/app/content_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/80001/content/public/app/conte... content/public/app/content_main_delegate.h:80: virtual bool ShouldEnableProfilerRecording() {return false;} Please mention the bug here too.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/999883002/diff/80001/content/public/app/conte... File content/public/app/content_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/80001/content/public/app/conte... content/public/app/content_main_delegate.h:80: virtual bool ShouldEnableProfilerRecording() {return false;} On 2015/03/13 15:27:41, vadimt wrote: > Please mention the bug here too. Done.
lgtm
yiyaoliu@chromium.org changed reviewers: + cpu@chromium.org
Hi Carlos, Could you do an owner's review? Thanks! Yiyao
ping?
https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:18: #include "base/profiler/scoped_tracker.h" why do we need this here? https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.h:45: bool ShouldEnableProfilerRecording() override; this function looks like it is in the middle of a bunch of override impl of a delegate.
https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:18: #include "base/profiler/scoped_tracker.h" On 2015/03/16 22:16:25, cpu wrote: > why do we need this here? Done. https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.h:45: bool ShouldEnableProfilerRecording() override; On 2015/03/16 22:16:25, cpu wrote: > this function looks like it is in the middle of a bunch of override impl of a > delegate. Sorry I don't quite understand your suggestion here. Are you saying this function should not be placed here? But it's in the same order with the base class ContentMainDelegate.
https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.h:45: bool ShouldEnableProfilerRecording() override; yao@, it looks like there are 2 sections of overrides, separated by an empty line. The first group is "just" overrides, and the second one is "Create*" overrides. Perhaps, you should move your method into the first group.
https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/999883002/diff/120001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.h:45: bool ShouldEnableProfilerRecording() override; On 2015/03/17 14:37:26, vadimt wrote: > yao@, it looks like there are 2 sections of overrides, separated by an empty > line. > > The first group is "just" overrides, and the second one is "Create*" overrides. > Perhaps, you should move your method into the first group. > Like this? (Also, The first half is public in the base class, and the second half is protected.)
Yes
lgtm
The CQ bit was checked by vadimt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org Link to the patchset: https://codereview.chromium.org/999883002/#ps160001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999883002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Changed the code a little bit to correct a format error. (A virtual function can't have inline definition, moved it to .cc file.) Will submit soon.
The CQ bit was checked by yiyaoliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org, vadimt@chromium.org Link to the patchset: https://codereview.chromium.org/999883002/#ps180001 (title: "Correct format error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999883002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9e6a5ab378cec3edf0eeba1b6867db055d32d2a7 Cr-Commit-Position: refs/heads/master@{#321166} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
