Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
957353003:
Add extra histogram latency tracking in PreMainMessageLoopRunImpl. (Closed)
DescriptionAdd extra histogram latency tracking in PreMainMessageLoopRunImpl.
BUG=454789
Committed: https://crrev.com/9c7eb4473688a74e0f2243a764e27323949fad36
Cr-Commit-Position: refs/heads/master@{#318806}
Patch Set 1 #Patch Set 2 : ws fixes #
Total comments: 4
Patch Set 3 : asvitkine comments and formatting #Patch Set 4 : wrap write of step3 in non-android #Patch Set 5 : fix comments #Patch Set 6 : fix xml comment #
Total comments: 9
Patch Set 7 : move step2 def to not_android only #Patch Set 8 : all all endif comments #
Total comments: 1
Patch Set 9 : remove spaces and add comment #Messages
Total messages: 22 (5 generated)
rkaplow@chromium.org changed reviewers: + asvitkine@chromium.org, thestig@chromium.org
thestig@chromium.org: Please review changes in asvitkine@chromium.org: Please review changes in
https://codereview.chromium.org/957353003/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/957353003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:1301: profile_ = CreatePrimaryProfile(parameters(), Add a comment that this function is covered by a histogram, else it looks like a bug that your histograms aren't covering it. https://codereview.chromium.org/957353003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:1559: bool started = browser_creator_->Start(parsed_command_line(), Ditto.
https://codereview.chromium.org/957353003/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/957353003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:1301: profile_ = CreatePrimaryProfile(parameters(), On 2015/02/26 20:58:39, Alexei Svitkine wrote: > Add a comment that this function is covered by a histogram, else it looks like a > bug that your histograms aren't covering it. Done. https://codereview.chromium.org/957353003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:1559: bool started = browser_creator_->Start(parsed_command_line(), On 2015/02/26 20:58:40, Alexei Svitkine wrote: > Ditto. Done.
lgtm
lgtm
The CQ bit was checked by rkaplow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957353003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
I missed that end of step2 was only defined under !android, so had to put the
writing of 3 under !android as well.
I put it under a seperate #if statement since it was unrelated to the
if (result_code_ <= 0) {
RecordBrowserStartupTime();
}
bit, and I thought putting as an else would be a bit more confusing. I'm fine
either way though if someone else has a preference.
I also added a bunch of #endif // ... to try to make it easier to see such
things. Let me know if you think it's a bit overboard.
https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1165: #endif // OS_WIN defined() https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1209: #endif // OS_POSIX && !defined(OS_MACOSX) Nit: defined(OS_POSIX) && !defined(OS_MACOSX) #if defined(FOO) is not the same as #if FOO better to be precise. https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1322: #endif // ENABLE_BACKGROUND defined() https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1416: #endif // ENABLE_RLZ && !defined(OS_CHROMEOS) Change back to defined() https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1459: #endif // ENABLE_PRINT_PREVIEW && !defined(OFFICIAL_BUILD) Ditto. https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1494: #endif // ENABLE_PRINT_PREVIEW Ditto. https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1504: #endif // OS_ANDROID Ditto. https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1557: #endif // OS_CHROMEOS Ditto.
Oh, so many #ifdefs. It was not at all from +/- 10 lines context. So should we measure Startup.PreMainMessageLoopRunImplStep2Time and Startup.PreMainMessageLoopRunImplStep3Time on Android too? https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1165: #endif // OS_WIN nit: Please be consistent and add defined(). Ditto below.
On 2015/03/02 22:22:29, Lei Zhang wrote: > Oh, so many #ifdefs. It was not at all from +/- 10 lines context. So should we > measure Startup.PreMainMessageLoopRunImplStep2Time and > Startup.PreMainMessageLoopRunImplStep3Time on Android too? > > https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/957353003/diff/100001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:1165: #endif // OS_WIN > nit: Please be consistent and add defined(). Ditto below. I've gone through the whole file and added #endif comments, although in this case it actually was there (ending the OS_ANDROID). I had missed it was under an !Android since that block was 100 lines long, and macros make it hard to see since there's no indenting to hint. I was not planning on adding Android specific ones since I suspect these new histograms will not be long lived, I will likely replace them with histograms at a deeper level once I figure out better where the costs are from, and I'm mostly looking at desktop numbers. So they wouldn't be helpful for me right now.
LGTM thanks for the cleanup, just one last comment (throughout), otherwise looks great! https://codereview.chromium.org/957353003/diff/140001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/957353003/diff/140001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:184: #endif // defined(OS_WIN) Nit: Why did you put two spaces after the // for these? Style guide says to put two spaces *before*, not after.
On 2015/03/02 22:33:11, rkaplow wrote: > I was not planning on adding Android specific ones since I suspect these new > histograms will not be long lived, I will likely replace them with histograms at > a deeper level once I figure out better where the costs are from, and I'm mostly > looking at desktop numbers. So they wouldn't be helpful for me right now. If they are not going to be there for too long, then I won't worry about it so much.
On 2015/03/02 22:35:51, Alexei Svitkine wrote: > LGTM thanks for the cleanup, just one last comment (throughout), otherwise looks > great! > > https://codereview.chromium.org/957353003/diff/140001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/957353003/diff/140001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:184: #endif // defined(OS_WIN) > Nit: Why did you put two spaces after the // for these? Style guide says to put > two spaces *before*, not after. Was not intentional there. I also added a comment explaining the IDS_USED_EXISTING_BROWSER bit since it seemed a bit mysterious we were doing a printf()
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/957353003/#ps160001 (title: "remove spaces and add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957353003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9c7eb4473688a74e0f2243a764e27323949fad36 Cr-Commit-Position: refs/heads/master@{#318806} |
