Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345933002:
Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib (Closed)
|
Created:
4 years, 3 months ago by grt (UTC plus 2) Modified:
4 years, 3 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBreak chrome_initial's dependence on //components/startup_metric_utils/browser:lib
We can't have chrome.exe depending on browser code since doing so can
unexpectedly cause the size of chrome.exe to bloat. This change
accomplishes the breakage by passing the main exe entry TimeTicks
through to chrome.dll directly by way of ChromeMain and
ChromeMainDelegate rather than via an environment variable.
BUG=647223
R=brucedawson@chromium.org
Committed: https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c
Cr-Commit-Position: refs/heads/master@{#419881}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixes #Patch Set 3 : more fixes #
Total comments: 17
Patch Set 4 : back in time #Patch Set 5 : ticks everywhere #
Total comments: 4
Patch Set 6 : fdoray feedback #
Total comments: 2
Messages
Total messages: 55 (35 generated)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This resolves the issue for Chromium builds for me, and I think is the right thing anyway. Whaddya think of my approach here? https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:10: #include <memory> unused
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:10: #include <memory> On 2016/09/16 09:38:26, grt (UTC plus 2) wrote: > unused Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This seems much cleaner, and safer. It also (fractionally) improves the linking times for chrome.exe, although not enough to matter. Just a few questions. https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:10: #include <memory> On 2016/09/16 12:21:38, grt (UTC plus 2) wrote: > On 2016/09/16 09:38:26, grt (UTC plus 2) wrote: > > unused > > Done. Talking to yourself again? https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_exe_m... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_win.cc:227: const base::TimeTicks exe_entry_point_ticks = base::TimeTicks::Now(); What was the reason for changing from Time to TimeTicks? https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:461: ChromeMainDelegate::ChromeMainDelegate() : ChromeMainDelegate(0) {} I hadn't realized that delegating constructors were approved. Cool. But, what is the advantage over having a default value for the argument?
Description was changed from ========== Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib. We can't have chrome.exe depending on browser code since doing so can unexpectedly cause the size of chrome.exe to bloat. This change accomplishes the breakage by passing the main exe entry TimeTicks through to chrome.dll directly by way of ChromeMain and ChromeMainDelegate rather than via an environment variable. BUG=647223 R=brucedawson@chromium.org ========== to ========== Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib We can't have chrome.exe depending on browser code since doing so can unexpectedly cause the size of chrome.exe to bloat. This change accomplishes the breakage by passing the main exe entry TimeTicks through to chrome.dll directly by way of ChromeMain and ChromeMainDelegate rather than via an environment variable. BUG=647223 R=brucedawson@chromium.org ==========
grt@chromium.org changed reviewers: + gab@chromium.org
+gab to review startup_metrics_utils. Please see my question in https://codereview.chromium.org/1425263003/#msg58. I sorta think that this change will potentially improve the startup metrics since there won't be two distinct Time->TimeTicks conversions, but I haven't looked at the code too closely. Thanks. https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2345933002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:10: #include <memory> On 2016/09/16 21:38:38, brucedawson wrote: > On 2016/09/16 12:21:38, grt (UTC plus 2) wrote: > > On 2016/09/16 09:38:26, grt (UTC plus 2) wrote: > > > unused > > > > Done. > > Talking to yourself again? I'm a fine conversationalist. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_exe_m... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_win.cc:227: const base::TimeTicks exe_entry_point_ticks = base::TimeTicks::Now(); On 2016/09/16 21:38:39, brucedawson wrote: > What was the reason for changing from Time to TimeTicks? Oops. I thought it was the right thing to do, but now that you've asked I see that startup_metrics_utils has a convoluted StartupTimeToTimeTicks that is now being bypassed. I'll ask the authors of that if it's necessary for that conversion to take place here and in chrome.dll. As it is, the calculations are done twice (once in chrome.exe and once in chrome.dll), so they are not guaranteed to have the same deltas. Thanks for questioning that! https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:461: ChromeMainDelegate::ChromeMainDelegate() : ChromeMainDelegate(0) {} On 2016/09/16 21:38:39, brucedawson wrote: > I hadn't realized that delegating constructors were approved. Cool. > > But, what is the advantage over having a default value for the argument? Default values for args are somewhat discouraged by the style guide. There's not a huge difference, I don't think.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
grt@chromium.org changed reviewers: + fdoray@chromium.org - gab@chromium.org
-gab +fdoray: Would you take a look at Patch Set 3? Thanks.
https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_exe_m... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_win.cc:227: const base::TimeTicks exe_entry_point_ticks = base::TimeTicks::Now(); On 2016/09/17 09:30:05, grt (UTC plus 2) wrote: > On 2016/09/16 21:38:39, brucedawson wrote: > > What was the reason for changing from Time to TimeTicks? > > Oops. I thought it was the right thing to do, but now that you've asked I see > that startup_metrics_utils has a convoluted StartupTimeToTimeTicks that is now > being bypassed. I'll ask the authors of that if it's necessary for that > conversion to take place here and in chrome.dll. As it is, the calculations are > done twice (once in chrome.exe and once in chrome.dll), so they are not > guaranteed to have the same deltas. Thanks for questioning that! This is the right thing to do. We should use base::TimeTicks whenever possible in startup_metric_utils. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main.... chrome/app/chrome_main.cc:27: int64_t exe_entry_point_ticks); #include <stdint.h>? https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main.... chrome/app/chrome_main.cc:27: int64_t exe_entry_point_ticks); Could it be base::TimeTicks instead of int64_t? Or that doesn't work with DLLEXPORT __cdecl? https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:437: void RecordMainStartupMetrics(int64_t exe_entry_point_ticks) { base::TimeTicks https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.h:28: explicit ChromeMainDelegate(int64_t exe_entry_point_ticks); // |exe_main_entry_time| is the time at which the main function // of the executable was entered (null if not available). explicit ChromeMainDelegate(const base::TimeTicks& exe_main_entry_time); https://codereview.chromium.org/2345933002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:53: typedef int (*DLL_MAIN)(HINSTANCE, sandbox::SandboxInterfaceInfo*, int64_t); base::TimeTicks? https://codereview.chromium.org/2345933002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2345933002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:650: g_browser_exe_main_entry_point_ticks.Get(); Either use g_browser_exe_main_entry_point_ticks.Get() directly below or also cache g_browser_main_entry_point_ticks.Get().
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Thanks. PTAL. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main.... chrome/app/chrome_main.cc:27: int64_t exe_entry_point_ticks); On 2016/09/19 17:03:28, fdoray wrote: > #include <stdint.h>? Done. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main.... chrome/app/chrome_main.cc:27: int64_t exe_entry_point_ticks); On 2016/09/19 17:03:28, fdoray wrote: > Could it be base::TimeTicks instead of int64_t? Or that doesn't work with > DLLEXPORT __cdecl? It's safest to stick to primitive types when crossing a boundary like this. Since we expect the two binaries to be built with the same compiler at the same time, it may be safe to use base::TimeTicks, but I'm inclined to be conservative. Feel free to ask Siggi what he thinks if you find the int64_t bit distasteful. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:437: void RecordMainStartupMetrics(int64_t exe_entry_point_ticks) { On 2016/09/19 17:03:28, fdoray wrote: > base::TimeTicks Done. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.h:28: explicit ChromeMainDelegate(int64_t exe_entry_point_ticks); On 2016/09/19 17:03:28, fdoray wrote: > // |exe_main_entry_time| is the time at which the main function > // of the executable was entered (null if not available). > explicit ChromeMainDelegate(const base::TimeTicks& exe_main_entry_time); Done. https://codereview.chromium.org/2345933002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/2345933002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:53: typedef int (*DLL_MAIN)(HINSTANCE, sandbox::SandboxInterfaceInfo*, int64_t); On 2016/09/19 17:03:28, fdoray wrote: > base::TimeTicks? See comment in chrome_main.cc. https://codereview.chromium.org/2345933002/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2345933002/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:650: g_browser_exe_main_entry_point_ticks.Get(); On 2016/09/19 17:03:28, fdoray wrote: > Either use g_browser_exe_main_entry_point_ticks.Get() directly below or also > cache g_browser_main_entry_point_ticks.Get(). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm w/ comments https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:440: startup_metric_utils::RecordExeMainEntryPointTicks(exe_entry_point_ticks); The #if is there because base::CurrentProcessInfo::CreationTime() isn't defined on Android. There is no reason to put RecordExeMainEntryPointTicks() inside the #if. |exe_entry_point_ticks| will be null on all platforms except Windows so you could surround RecordExeMainEntryPointTicks() with #if defined(OS_WIN). However, I would not do that because it would force us to modify this file when we add exe entry point time on Mac (we plan to do it soon). https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.h:30: explicit ChromeMainDelegate(int64_t exe_entry_point_ticks); I understand why it's more conservative to use int64_t in ChromeMain, but here I would use base::TimeTicks. I prefer to put the value back in a TimeTicks as soon as possible to minimize the amount of code where it's ambiguous how time is represented.
Patchset #6 (id:120001) has been deleted
Thanks for the great comments. https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:440: startup_metric_utils::RecordExeMainEntryPointTicks(exe_entry_point_ticks); On 2016/09/20 12:25:21, fdoray wrote: > The #if is there because base::CurrentProcessInfo::CreationTime() isn't defined > on Android. There is no reason to put RecordExeMainEntryPointTicks() inside the > #if. > > |exe_entry_point_ticks| will be null on all platforms except Windows so you > could surround RecordExeMainEntryPointTicks() with #if defined(OS_WIN). However, > I would not do that because it would force us to modify this file when we add > exe entry point time on Mac (we plan to do it soon). Done. https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.h (right): https://codereview.chromium.org/2345933002/diff/100001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.h:30: explicit ChromeMainDelegate(int64_t exe_entry_point_ticks); On 2016/09/20 12:25:22, fdoray wrote: > I understand why it's more conservative to use int64_t in ChromeMain, but here I > would use base::TimeTicks. I prefer to put the value back in a TimeTicks as soon > as possible to minimize the amount of code where it's ambiguous how time is > represented. I like it. Done. https://codereview.chromium.org/2345933002/diff/140001/chrome/app/main_dll_lo... File chrome/app/main_dll_loader_win.h (right): https://codereview.chromium.org/2345933002/diff/140001/chrome/app/main_dll_lo... chrome/app/main_dll_loader_win.h:34: int Launch(HINSTANCE instance, base::TimeTicks exe_entry_point_ticks); n.b.: I changed this to pass-by-value rather than by-reference based on this comment in time.h: // All time classes are copyable, assignable, and occupy 64-bits per // instance. Thus, they can be efficiently passed by-value (as opposed to // by-reference). If you revisit startup_metric_utils, I propose that you change it from by-ref to by-value. https://codereview.chromium.org/2345933002/diff/140001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/2345933002/diff/140001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.h:48: void RecordExeMainEntryPointTicks(const base::TimeTicks& time); I left this as const-ref for consistency with the surrounding code.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2345933002/#ps140001 (title: "fdoray feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm++
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib We can't have chrome.exe depending on browser code since doing so can unexpectedly cause the size of chrome.exe to bloat. This change accomplishes the breakage by passing the main exe entry TimeTicks through to chrome.dll directly by way of ChromeMain and ChromeMainDelegate rather than via an environment variable. BUG=647223 R=brucedawson@chromium.org ========== to ========== Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib We can't have chrome.exe depending on browser code since doing so can unexpectedly cause the size of chrome.exe to bloat. This change accomplishes the breakage by passing the main exe entry TimeTicks through to chrome.dll directly by way of ChromeMain and ChromeMainDelegate rather than via an environment variable. BUG=647223 R=brucedawson@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib We can't have chrome.exe depending on browser code since doing so can unexpectedly cause the size of chrome.exe to bloat. This change accomplishes the breakage by passing the main exe entry TimeTicks through to chrome.dll directly by way of ChromeMain and ChromeMainDelegate rather than via an environment variable. BUG=647223 R=brucedawson@chromium.org ========== to ========== Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib We can't have chrome.exe depending on browser code since doing so can unexpectedly cause the size of chrome.exe to bloat. This change accomplishes the breakage by passing the main exe entry TimeTicks through to chrome.dll directly by way of ChromeMain and ChromeMainDelegate rather than via an environment variable. BUG=647223 R=brucedawson@chromium.org Committed: https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c Cr-Commit-Position: refs/heads/master@{#419881} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c Cr-Commit-Position: refs/heads/master@{#419881} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
