Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1425263003:
Use TimeTicks as much as possible in startup_metric_utils. (Closed)
DescriptionUse TimeTicks as much as possible in startup_metric_utils.
BUG=544131, 521164
Committed: https://crrev.com/4768aeea7babd463eae04fc7a51f318bad812885
Cr-Commit-Position: refs/heads/master@{#359183}
Patch Set 1 #
Total comments: 8
Patch Set 2 : comment #6 #Patch Set 3 : merge up to r358781 #Patch Set 4 : fix mac compile #
Total comments: 2
Patch Set 5 : adapt mojo as well + nit #
Total comments: 6
Patch Set 6 : msw nits #Patch Set 7 : ifdef thread priority dance out on Mac per lack of support in base #
Total comments: 2
Messages
Total messages: 62 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
gab@chromium.org changed reviewers: + fdoray@chromium.org
Francois PTAL :-). Thanks! Gab
lgtm with comments https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:259: // is temporarily increased to avoid this thread by descheduled between the two this thread *to be* descheduled https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:266: // TODO(jeremy): Remove this when resolving crbug.com/317481. https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:366: env->SetVar(kChromeMainTicksEnvVar, exe_load_ticks); It might be good to explain that we need an environment variable to store this value because static variables aren't shared between chrome.exe and chrome.dll, near the definition of kChromeMainTicksEnvVar. https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:14: // received as Time but are converted to TimeTicks as soon as possible as well The "as well" sounds weird to me. But I trust you English skills better than mine.
Thanks, done (except for a TODO request which I didn't understand). https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:259: // is temporarily increased to avoid this thread by descheduled between the two On 2015/11/09 21:27:16, fdoray wrote: > this thread *to be* descheduled Also re-worded (kept priority details for next comment. https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:266: On 2015/11/09 21:27:16, fdoray wrote: > // TODO(jeremy): Remove this when resolving crbug.com/317481. Why? This is used to only do the priority dance when initializing statics, has nothing to do with remaining use of base::Time. https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:366: env->SetVar(kChromeMainTicksEnvVar, exe_load_ticks); On 2015/11/09 21:27:16, fdoray wrote: > It might be good to explain that we need an environment variable to store this > value because static variables aren't shared between chrome.exe and chrome.dll, > near the definition of kChromeMainTicksEnvVar. Done. https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/1425263003/diff/60001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.h:14: // received as Time but are converted to TimeTicks as soon as possible as well On 2015/11/09 21:27:16, fdoray wrote: > The "as well" sounds weird to me. But I trust you English skills better than > mine. Agreed, re-worded entire comment.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
gab@chromium.org changed reviewers: + thakis@chromium.org
@Nico for OWNERS on side-effects in chrome/browser/* Thanks, Gab
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/100001
Description was changed from ========== Use TraceTicks as much as possible in startup_metric_utils. BUG=544131 ========== to ========== Use TimeTicks as much as possible in startup_metric_utils. BUG=544131 ==========
Description was changed from ========== Use TimeTicks as much as possible in startup_metric_utils. BUG=544131 ========== to ========== Use TimeTicks as much as possible in startup_metric_utils. BUG=544131, 521164 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chrome/ lgtm https://codereview.chromium.org/1425263003/diff/120001/chrome/browser/mac/mac... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/1425263003/diff/120001/chrome/browser/mac/mac... chrome/browser/mac/mac_startup_profiler.cc:36: it != profiled_ticks_.end(); ++it) { nit: since you're touching this anyways, consider for (const auto& entry : profiled_ticks) { const base::TimeTicks& location_ticks = entry.first; ...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/140001
Patchset #5 (id:140001) has been deleted
gab@chromium.org changed reviewers: + msw@chromium.org, sky@chromium.org
@msw for html_viewer @sky for mandoline/ and mojo/ Thanks, Gab https://codereview.chromium.org/1425263003/diff/120001/chrome/browser/mac/mac... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/1425263003/diff/120001/chrome/browser/mac/mac... chrome/browser/mac/mac_startup_profiler.cc:36: it != profiled_ticks_.end(); ++it) { On 2015/11/10 05:15:50, Nico wrote: > nit: since you're touching this anyways, consider > > for (const auto& entry : profiled_ticks) { > const base::TimeTicks& location_ticks = entry.first; > ... Done.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM
lgtm with nits and q/+TODO(msw) https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... chrome/browser/mac/mac_startup_profiler.cc:35: const Location location = entry.first; nit: could this also be a const ref? (or inline [both and remove curlies]?) https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... chrome/browser/mac/mac_startup_profiler.h:53: // Keeps track of the TimeTicks at which each initialization phase was nit: the comment change isn't really necessary. https://codereview.chromium.org/1425263003/diff/160001/mojo/services/tracing/... File mojo/services/tracing/public/interfaces/tracing.mojom (right): https://codereview.chromium.org/1425263003/diff/160001/mojo/services/tracing/... mojo/services/tracing/public/interfaces/tracing.mojom:36: int64 shell_process_creation_time; Hmm, I guess we might want to convert these times to ticks earlier, then add an accessor to set the ticks instead of times in startup_metric_utils? (rather than holding the time here for a while, then passing to startup_metric_utils when the value is needed and only converting then... given the discussion around converting as early as possible in crbug.com/544131) Give me a TODO?
Thanks, comments addressed, sending to CQ. https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... chrome/browser/mac/mac_startup_profiler.cc:35: const Location location = entry.first; On 2015/11/10 18:58:37, msw wrote: > nit: could this also be a const ref? (or inline [both and remove curlies]?) It's an enum, refs for enums don't make much sense. I don't think the types add clarity though I guess that makes a case for not using auto... Changed for explicit type, with one line for-each loop. https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/1425263003/diff/160001/chrome/browser/mac/mac... chrome/browser/mac/mac_startup_profiler.h:53: // Keeps track of the TimeTicks at which each initialization phase was On 2015/11/10 18:58:38, msw wrote: > nit: the comment change isn't really necessary. Done. https://codereview.chromium.org/1425263003/diff/160001/mojo/services/tracing/... File mojo/services/tracing/public/interfaces/tracing.mojom (right): https://codereview.chromium.org/1425263003/diff/160001/mojo/services/tracing/... mojo/services/tracing/public/interfaces/tracing.mojom:36: int64 shell_process_creation_time; On 2015/11/10 18:58:38, msw wrote: > Hmm, I guess we might want to convert these times to ticks earlier, then add an > accessor to set the ticks instead of times in startup_metric_utils? (rather than > holding the time here for a while, then passing to startup_metric_utils when the > value is needed and only converting then... given the discussion around > converting as early as possible in crbug.com/544131) Give me a TODO? Right having the TimeTicks value ASAP is better, however for the startup_metrics_util API I was trying to make an API that (1) works on every platform (some of which don't know that value in TimeTicks) and (2) embeds the Time -> TimeTicks transformation. Happy to add a TODO for you to think of a better way, but I'm not sure about having two entry points on startup_metrics_util.h.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, thakis@chromium.org, msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1425263003/#ps180001 (title: "msw nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:200001) has been deleted
On 2015/11/11 17:20:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Couldn't think this was possibly related, but finally found the issue. Turns out base APIs for thread priority aren't properly supported on Mac... ifdef'ed out the thread priority dance on Mac -- no worse than before I guess -- and filed crbug.com/554651 to find someone to fix this for Mac Cheers, Gab
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, fdoray@chromium.org, thakis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1425263003/#ps220001 (title: "ifdef thread priority dance out on Mac per lack of support in base")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425263003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425263003/220001
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4768aeea7babd463eae04fc7a51f318bad812885 Cr-Commit-Position: refs/heads/master@{#359183}
Message was sent while issue was closed.
grt@chromium.org changed reviewers: + grt@chromium.org
Message was sent while issue was closed.
Question for you, Gab, relating to what I'm trying to fix in https://codereview.chromium.org/2345933002/. Thanks for taking a look. https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:269: static bool statics_initialized = false; Hi gang. The call to RecordExeMainEntryPointTime from chrome.exe is problematic since it results in chrome.exe having a dependence on many browser-level libs. This recently led to an accidental 150% size increase in chrome.exe. In trying to resolve this, I naively grabbed TimeTicks in wWinMain instead of Time. After brucedawson asked why I did that, I noticed this code here. Are you aware that there will be two copies of these statics in the process: one in chrome.exe (initialized when RecordExeMainEntryPointTime is called) and one in chrome.dll (initialized when RecordStartupProcessCreationTime is called)? Is this a problem? Will the right thing happen if I pass an internal base::Time value from wWinMain into ChromeMain, and then do the Time -> TimeTicks conversion for the ExeMainEntryPointTime there?
Message was sent while issue was closed.
https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:269: static bool statics_initialized = false; On 2016/09/17 09:22:06, grt (UTC plus 2) wrote: > Hi gang. The call to RecordExeMainEntryPointTime from chrome.exe is problematic > since it results in chrome.exe having a dependence on many browser-level libs. > This recently led to an accidental 150% size increase in chrome.exe. In trying > to resolve this, I naively grabbed TimeTicks in wWinMain instead of Time. After > brucedawson asked why I did that, I noticed this code here. Are you aware that > there will be two copies of these statics in the process: one in chrome.exe > (initialized when RecordExeMainEntryPointTime is called) and one in chrome.dll > (initialized when RecordStartupProcessCreationTime is called)? Is this a > problem? > > Will the right thing happen if I pass an internal base::Time value from wWinMain > into ChromeMain, and then do the Time -> TimeTicks conversion for the > ExeMainEntryPointTime there? RecordStartupProcessCreationTime() takes a base::Time as argument because the process creation time is only available as base::Time https://cs.chromium.org/chromium/src/base/process/process_info.h?sq=package:c... , but there is no good reason why RecordExeMainEntryPointTime() takes a base::Time as argument and converts it to base::Time instead of taking a base::TimeTicks directly [1]. [1] Maybe a call to StartupTimeToTimeTicks was kept in RecordExeMainEntryPointTime to initialize the static variable below? To make the conversion of the process creation time from base::Time to base::TimeTicks as accurate as possible, these variables must be initialized as soon as possible after process creation. Given that there is a different copy of these variables in chrome.exe/chrome.dll, it's really not useful to keep a call to StartupTimeToTimeTicks in RecordExeMainEntryPointTime. My recommendation: 1. Change RecordExeMainEntryPointTime() to take a TimeTicks as argument. 2. Capture a TimeTicks on exe main entry. Forward it to ChromeMain() and call RecordExeMainEntryPointTime() in ChromeMain. 3. Remove the dependency from chrome.exe on startup_metric_utils. 4. Capture base::Time/base::TimeTicks early in chrome.exe main. Forward the values to ChromeMain() and use them to do the conversion of the process creation time from base::Time to base::TimeTicks. (gab@ or me could take care of this).
Message was sent while issue was closed.
On 2016/09/19 13:14:46, fdoray wrote: > https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... > File components/startup_metric_utils/browser/startup_metric_utils.cc (right): > > https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... > components/startup_metric_utils/browser/startup_metric_utils.cc:269: static bool > statics_initialized = false; > On 2016/09/17 09:22:06, grt (UTC plus 2) wrote: > > Hi gang. The call to RecordExeMainEntryPointTime from chrome.exe is > problematic > > since it results in chrome.exe having a dependence on many browser-level libs. > > This recently led to an accidental 150% size increase in chrome.exe. In trying > > to resolve this, I naively grabbed TimeTicks in wWinMain instead of Time. > After > > brucedawson asked why I did that, I noticed this code here. Are you aware that > > there will be two copies of these statics in the process: one in chrome.exe > > (initialized when RecordExeMainEntryPointTime is called) and one in chrome.dll > > (initialized when RecordStartupProcessCreationTime is called)? Is this a > > problem? > > > > Will the right thing happen if I pass an internal base::Time value from > wWinMain > > into ChromeMain, and then do the Time -> TimeTicks conversion for the > > ExeMainEntryPointTime there? > > RecordStartupProcessCreationTime() takes a base::Time as argument because the > process creation time is only available as base::Time > https://cs.chromium.org/chromium/src/base/process/process_info.h?sq=package:c... > , but there is no good reason why RecordExeMainEntryPointTime() takes a > base::Time as argument and converts it to base::Time instead of taking a > base::TimeTicks directly [1]. > > [1] Maybe a call to StartupTimeToTimeTicks was kept in > RecordExeMainEntryPointTime to initialize the static variable below? To make the > conversion of the process creation time from base::Time to base::TimeTicks as > accurate as possible, these variables must be initialized as soon as possible > after process creation. Given that there is a different copy of these variables > in chrome.exe/chrome.dll, it's really not useful to keep a call to > StartupTimeToTimeTicks in RecordExeMainEntryPointTime. > > My recommendation: > 1. Change RecordExeMainEntryPointTime() to take a TimeTicks as argument. > 2. Capture a TimeTicks on exe main entry. Forward it to ChromeMain() and call > RecordExeMainEntryPointTime() in ChromeMain. > 3. Remove the dependency from chrome.exe on startup_metric_utils. > 4. Capture base::Time/base::TimeTicks early in chrome.exe main. Forward the > values to ChromeMain() and use them to do the conversion of the process creation > time from base::Time to base::TimeTicks. (gab@ or me could take care of this). I did 1-3 in https://codereview.chromium.org/2345933002/#ps60001. Would you mind taking a look at that to see what you think? In Patch Set 4 I switched back to base::Time, so disregard that. Is it "safe" for me to land Patch Set 3 before you or gab@ do #4?
Message was sent while issue was closed.
On 2016/09/19 13:57:57, grt (UTC plus 2) wrote: > On 2016/09/19 13:14:46, fdoray wrote: > > > https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... > > File components/startup_metric_utils/browser/startup_metric_utils.cc (right): > > > > > https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... > > components/startup_metric_utils/browser/startup_metric_utils.cc:269: static > bool > > statics_initialized = false; > > On 2016/09/17 09:22:06, grt (UTC plus 2) wrote: > > > Hi gang. The call to RecordExeMainEntryPointTime from chrome.exe is > > problematic > > > since it results in chrome.exe having a dependence on many browser-level > libs. > > > This recently led to an accidental 150% size increase in chrome.exe. In > trying > > > to resolve this, I naively grabbed TimeTicks in wWinMain instead of Time. > > After > > > brucedawson asked why I did that, I noticed this code here. Are you aware > that > > > there will be two copies of these statics in the process: one in chrome.exe > > > (initialized when RecordExeMainEntryPointTime is called) and one in > chrome.dll > > > (initialized when RecordStartupProcessCreationTime is called)? Is this a > > > problem? > > > > > > Will the right thing happen if I pass an internal base::Time value from > > wWinMain > > > into ChromeMain, and then do the Time -> TimeTicks conversion for the > > > ExeMainEntryPointTime there? > > > > RecordStartupProcessCreationTime() takes a base::Time as argument because the > > process creation time is only available as base::Time > > > https://cs.chromium.org/chromium/src/base/process/process_info.h?sq=package:c... > > , but there is no good reason why RecordExeMainEntryPointTime() takes a > > base::Time as argument and converts it to base::Time instead of taking a > > base::TimeTicks directly [1]. > > > > [1] Maybe a call to StartupTimeToTimeTicks was kept in > > RecordExeMainEntryPointTime to initialize the static variable below? To make > the > > conversion of the process creation time from base::Time to base::TimeTicks as > > accurate as possible, these variables must be initialized as soon as possible > > after process creation. Given that there is a different copy of these > variables > > in chrome.exe/chrome.dll, it's really not useful to keep a call to > > StartupTimeToTimeTicks in RecordExeMainEntryPointTime. > > > > My recommendation: > > 1. Change RecordExeMainEntryPointTime() to take a TimeTicks as argument. > > 2. Capture a TimeTicks on exe main entry. Forward it to ChromeMain() and call > > RecordExeMainEntryPointTime() in ChromeMain. > > 3. Remove the dependency from chrome.exe on startup_metric_utils. > > 4. Capture base::Time/base::TimeTicks early in chrome.exe main. Forward the > > values to ChromeMain() and use them to do the conversion of the process > creation > > time from base::Time to base::TimeTicks. (gab@ or me could take care of this). > > I did 1-3 in https://codereview.chromium.org/2345933002/#ps60001. Would you mind > taking a look at that to see what you think? In Patch Set 4 I switched back to > base::Time, so disregard that. Is it "safe" for me to land Patch Set 3 before > you or gab@ do #4? Francois knows this code better than I (and looks like he's an OWNER now https://codereview.chromium.org/2352693002/ ;-)), so whatever he says!
Message was sent while issue was closed.
On 2016/09/19 13:57:57, grt (UTC plus 2) wrote: > On 2016/09/19 13:14:46, fdoray wrote: > > > https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... > > File components/startup_metric_utils/browser/startup_metric_utils.cc (right): > > > > > https://codereview.chromium.org/1425263003/diff/220001/components/startup_met... > > components/startup_metric_utils/browser/startup_metric_utils.cc:269: static > bool > > statics_initialized = false; > > On 2016/09/17 09:22:06, grt (UTC plus 2) wrote: > > > Hi gang. The call to RecordExeMainEntryPointTime from chrome.exe is > > problematic > > > since it results in chrome.exe having a dependence on many browser-level > libs. > > > This recently led to an accidental 150% size increase in chrome.exe. In > trying > > > to resolve this, I naively grabbed TimeTicks in wWinMain instead of Time. > > After > > > brucedawson asked why I did that, I noticed this code here. Are you aware > that > > > there will be two copies of these statics in the process: one in chrome.exe > > > (initialized when RecordExeMainEntryPointTime is called) and one in > chrome.dll > > > (initialized when RecordStartupProcessCreationTime is called)? Is this a > > > problem? > > > > > > Will the right thing happen if I pass an internal base::Time value from > > wWinMain > > > into ChromeMain, and then do the Time -> TimeTicks conversion for the > > > ExeMainEntryPointTime there? > > > > RecordStartupProcessCreationTime() takes a base::Time as argument because the > > process creation time is only available as base::Time > > > https://cs.chromium.org/chromium/src/base/process/process_info.h?sq=package:c... > > , but there is no good reason why RecordExeMainEntryPointTime() takes a > > base::Time as argument and converts it to base::Time instead of taking a > > base::TimeTicks directly [1]. > > > > [1] Maybe a call to StartupTimeToTimeTicks was kept in > > RecordExeMainEntryPointTime to initialize the static variable below? To make > the > > conversion of the process creation time from base::Time to base::TimeTicks as > > accurate as possible, these variables must be initialized as soon as possible > > after process creation. Given that there is a different copy of these > variables > > in chrome.exe/chrome.dll, it's really not useful to keep a call to > > StartupTimeToTimeTicks in RecordExeMainEntryPointTime. > > > > My recommendation: > > 1. Change RecordExeMainEntryPointTime() to take a TimeTicks as argument. > > 2. Capture a TimeTicks on exe main entry. Forward it to ChromeMain() and call > > RecordExeMainEntryPointTime() in ChromeMain. > > 3. Remove the dependency from chrome.exe on startup_metric_utils. > > 4. Capture base::Time/base::TimeTicks early in chrome.exe main. Forward the > > values to ChromeMain() and use them to do the conversion of the process > creation > > time from base::Time to base::TimeTicks. (gab@ or me could take care of this). > > I did 1-3 in https://codereview.chromium.org/2345933002/#ps60001. Would you mind > taking a look at that to see what you think? In Patch Set 4 I switched back to > base::Time, so disregard that. Is it "safe" for me to land Patch Set 3 before > you or gab@ do #4? It is "safe" to do 1-3 before we do 4. Because there are different copies of the static variables in chrome.exe/chrome.dll, doing 1-3 won't change the time at which we initialize the copies used to convert process creation time from Time to TimeTicks. |
