Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1610733002:
Store PreRead options obtained from the registry in a global variable. (Closed)
DescriptionStore PreRead options obtained from the registry in a global variable.
Instead of reading PreRead options from the registry every time they
are needed, read them once and store them in a global variable.
This optimization isn't immediately useful as PreRead options are
currently read only once per process. However, with the upcoming
/prefetch:# experiment, the browser process will need the PreRead
options every time it launches a process.
This CL also has these benefits:
- The caller of GetPreReadOptions doesn't need to know the product
registry path. This will allow content to get the PreRead options
to decide whether it should use a /prefetch:# switch when
launching a process.
- The PreRead options are returned as a bit field instead of by
setting multiple booleans. This will facilitate the addition of
new PreRead options.
BUG=577698
Committed: https://crrev.com/dee064299d3b3a13d5cc35f219d87d6a8478f227
Cr-Commit-Position: refs/heads/master@{#370805}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments from gab #5 #Patch Set 3 : self-review #Patch Set 4 : self-review #
Total comments: 17
Patch Set 5 : address comments from grt #8 #Patch Set 6 : self-review #Patch Set 7 : don't return a const ref #
Dependent Patchsets: Messages
Total messages: 21 (8 generated)
Description was changed from ========== Don't require a registry path in GetPreReadOptions(). This will allow content to read the PreRead field trial options without knowing about the product registry path. To assess the impact of adding a valid /prefetch switch, we'll need to be able to read PreRead options from content. BUG=577698 ========== to ========== Don't require a registry path in GetPreReadOptions(). This will allow content to read the PreRead field trial options without knowing about the product registry path. To assess the impact of adding a valid /prefetch switch, we'll need to be able to read PreRead options from content. Also, with this CL, PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org, grt@chromium.org
gab@ and grt@: Can you review this CL? Thanks.
Description was changed from ========== Don't require a registry path in GetPreReadOptions(). This will allow content to read the PreRead field trial options without knowing about the product registry path. To assess the impact of adding a valid /prefetch switch, we'll need to be able to read PreRead options from content. Also, with this CL, PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ========== to ========== Don't require a registry path in GetPreReadOptions(). This will allow content to read the PreRead field trial options without knowing about the product registry path. To assess the impact of using valid /prefetch switches, we'll need to be able to read PreRead options from content. Also, with this CL, PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ==========
Comments below. I'd say change title/description: to me this is more about early one-time optimization than about not passing the registry path later (which is also a plus but not what's currently broken). https://codereview.chromium.org/1610733002/diff/1/chrome/app/main_dll_loader_... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/1/chrome/app/main_dll_loader_... chrome/app/main_dll_loader_win.cc:67: startup_metric_utils::InitializePreReadOptions( This makes me think that this is/was wrong, later renderers will read the updated registry value and report being in the group meant for the next startup... Since the browser passes field trials to the renderers, here we should probably inspect existing field trials and use the Synthetic one instead of reading from registry? https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc (right): https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:43: bool g_pre_read_options_initialized = false; I'd like to avoid this global in non-dcheck builds. We could set |g_pre_read_options| to -1 when uninitialized and get rid of |g_pre_read_options_initialized|? Or instead of -1 (which indicates that everything is on if this is somehow returned accidentally in non-dcheck builds), we could have PRE_READ_OPTION_UNINITIALIZED in the enum and initialize to that..? (other option is to ifdef this and its usage to "#if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON))" but I prefer just not having the variable) https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:80: DCHECK(value == 1); DCHECK_EQ(1, value) https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.h (right): https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:42: // Returns the DLL pre-reading options to use for the current process. Returns the bitfield of the DLL pre-reading options... (i.e. mention it's a bitfield)
Description was changed from ========== Don't require a registry path in GetPreReadOptions(). This will allow content to read the PreRead field trial options without knowing about the product registry path. To assess the impact of using valid /prefetch switches, we'll need to be able to read PreRead options from content. Also, with this CL, PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ========== to ========== Store PreRead options obtained from the registry in a global variable. Instead of reading PreRead options from the registry every time they are needed, read them once and store them in a global variable. This optimization isn't immediately useful as PreRead options are currently read only once per process. However, with the upcoming /prefetch:# experiment, the browser process will need the PreRead options every time it launches a process. This CL also has these benefits: - The caller of GetPreReadOptions doesn't need to know the product registry path. This will allow content to get the PreRead options to decide whether it should use a /prefetch:# switch when launching a process. - The PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ==========
gab@: Can you take another look? grt@: Can you review this CL? https://codereview.chromium.org/1610733002/diff/1/chrome/app/main_dll_loader_... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/1/chrome/app/main_dll_loader_... chrome/app/main_dll_loader_win.cc:67: startup_metric_utils::InitializePreReadOptions( On 2016/01/20 20:09:39, gab wrote: > This makes me think that this is/was wrong, later renderers will read the > updated registry value and report being in the group meant for the next > startup... > > Since the browser passes field trials to the renderers, here we should probably > inspect existing field trials and use the Synthetic one instead of reading from > registry? As we discussed, this isn't problematic as long as we don't look at the metrics of old groups. https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc (right): https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:43: bool g_pre_read_options_initialized = false; On 2016/01/20 20:09:39, gab wrote: > I'd like to avoid this global in non-dcheck builds. > > We could set |g_pre_read_options| to -1 when uninitialized and get rid of > |g_pre_read_options_initialized|? > > Or instead of -1 (which indicates that everything is on if this is somehow > returned accidentally in non-dcheck builds), we could have > PRE_READ_OPTION_UNINITIALIZED in the enum and initialize to that..? > > (other option is to ifdef this and its usage to "#if (!defined(NDEBUG) || > defined(DCHECK_ALWAYS_ON))" but I prefer just not having the variable) Done, I added a PRE_READ_OPTION_UNINITIALIZED bit. Note: Since startup_metric_utils_common is a static library, there is multiple versions of g_pre_read_options in a component build. Therefore, I was forced to remove the DCHECK at the beginning of GetPreReadOptions. One way to avoid this problem would be to make startup_metric_utils_common a dynamic library... but I don't think it's worth it just to allow a DCHECK. https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:80: DCHECK(value == 1); On 2016/01/20 20:09:40, gab wrote: > DCHECK_EQ(1, value) Done. https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.h (right): https://codereview.chromium.org/1610733002/diff/1/components/startup_metric_u... components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:42: // Returns the DLL pre-reading options to use for the current process. On 2016/01/20 20:09:40, gab wrote: > Returns the bitfield of the DLL pre-reading options... > > (i.e. mention it's a bitfield) Done.
https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { i think this would be easier to read if pre_read_option was actually something like: struct PreReadOptions { bool no_pre_read : 1; bool high_priority : 1; ... }; wdyt? https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:40: int g_pre_read_options = PRE_READ_OPTION_UNINITIALIZED; if i'm reading the build files correctly, this .cc file is linked into both chrome.exe and chrome.dll. there will be one instance of this global variable in the process for each of these two, so InitializePreReadOptions will need to be called once from within each module. how will this be done? https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:52: DCHECK_EQ(g_pre_read_options, PRE_READ_OPTION_UNINITIALIZED); nit: always put the expected value first when using DCHECK_{EQ,NE} for consistency with Google Test's {EXPECT,ASSERT}_EQ macro, which requires it. https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:63: struct VariationMapping { nit: make this an anonymous struct since the name is unused https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:80: g_pre_read_options &= mapping.bit; |= https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.h (right): https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:25: PRE_READ_OPTION_NO_PRE_READ = 1 << 0, use a series of "const uint32_t kFooTheBar = n;" values rather than an enum for bits in a bitfield. that said, i think using a struct, as mentioned earlier, may be cleaner. https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:47: int GetPreReadOptions(); use uint32_t rather than int for the bitfield. bit twiddling on unsigned ints is more obvious than signed. that said, maybe the struct-based bitfield is cleaner.
grt@: Can you take another look? Thanks. https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { On 2016/01/21 04:24:00, grt wrote: > i think this would be easier to read if pre_read_option was actually something > like: > struct PreReadOptions { > bool no_pre_read : 1; > bool high_priority : 1; > ... > }; > wdyt? Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't be possible to take the address of members like I'm doing now in InitializePreReadOptions [1]. If you really want me to use :1, I could change InitializePreReadOptions so that it doesn't need to take the address of members. [1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:40: int g_pre_read_options = PRE_READ_OPTION_UNINITIALIZED; On 2016/01/21 04:24:00, grt wrote: > if i'm reading the build files correctly, this .cc file is linked into both > chrome.exe and chrome.dll. there will be one instance of this global variable in > the process for each of these two, so InitializePreReadOptions will need to be > called once from within each module. how will this be done? Yes. The call in chrome.exe is added by this CL here [1]. The call in chrome.dll will be added by another CL [2]. [1] https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... [2] https://codereview.chromium.org/1612663002/diff/1/chrome/browser/chrome_brows... https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:52: DCHECK_EQ(g_pre_read_options, PRE_READ_OPTION_UNINITIALIZED); On 2016/01/21 04:24:00, grt wrote: > nit: always put the expected value first when using DCHECK_{EQ,NE} for > consistency with Google Test's {EXPECT,ASSERT}_EQ macro, which requires it. Done (I removed this line). https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:63: struct VariationMapping { On 2016/01/21 04:24:00, grt wrote: > nit: make this an anonymous struct since the name is unused Done. https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:80: g_pre_read_options &= mapping.bit; On 2016/01/21 04:24:00, grt wrote: > |= Done (now using a struct). https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... File components/startup_metric_utils/common/pre_read_field_trial_utils_win.h (right): https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:25: PRE_READ_OPTION_NO_PRE_READ = 1 << 0, On 2016/01/21 04:24:00, grt wrote: > use a series of "const uint32_t kFooTheBar = n;" values rather than an enum for > bits in a bitfield. that said, i think using a struct, as mentioned earlier, may > be cleaner. Done (now using a struct). https://codereview.chromium.org/1610733002/diff/60001/components/startup_metr... components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:47: int GetPreReadOptions(); On 2016/01/21 04:24:00, grt wrote: > use uint32_t rather than int for the bitfield. bit twiddling on unsigned ints is > more obvious than signed. that said, maybe the struct-based bitfield is cleaner. Done (now using a struct).
lgtm provided that the options struct is returned by value rather than by constref. https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { On 2016/01/21 16:04:16, fdoray wrote: > On 2016/01/21 04:24:00, grt wrote: > > i think this would be easier to read if pre_read_option was actually something > > like: > > struct PreReadOptions { > > bool no_pre_read : 1; > > bool high_priority : 1; > > ... > > }; > > wdyt? > > Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't be > possible to take the address of members like I'm doing now in > InitializePreReadOptions [1]. If you really want me to use :1, I could change > InitializePreReadOptions so that it doesn't need to take the address of members. > > [1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx I find the data-driven loop in InitializePreReadOptions interesting, but not so compelling that it should dictate the overall interface. Four calls to ReadDW (or a tiny helper function) is quite clear, whereas the loop takes a little reading to validate. I'm okay with it as-is, just pointing out that a clean interface is more important than a clever implementation. :-) If you'd like to keep the loop, then avoiding the bitfield is fine. In either case, I think the struct should be passed by value rather than by reference. The compiler is potentially smart enough to pack the four bools into a 32-bit int.
lgtm Too bad that we can't DCHECK for initialization. I guess this also means that this code doesn't work in component builds? (that's a pain for testing isn't it?) https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { On 2016/01/21 18:15:42, grt wrote: > On 2016/01/21 16:04:16, fdoray wrote: > > On 2016/01/21 04:24:00, grt wrote: > > > i think this would be easier to read if pre_read_option was actually > something > > > like: > > > struct PreReadOptions { > > > bool no_pre_read : 1; > > > bool high_priority : 1; > > > ... > > > }; > > > wdyt? > > > > Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't be > > possible to take the address of members like I'm doing now in > > InitializePreReadOptions [1]. If you really want me to use :1, I could change > > InitializePreReadOptions so that it doesn't need to take the address of > members. > > > > [1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx > > I find the data-driven loop in InitializePreReadOptions interesting, but not so > compelling that it should dictate the overall interface. Four calls to ReadDW > (or a tiny helper function) is quite clear, whereas the loop takes a little > reading to validate. I'm okay with it as-is, just pointing out that a clean > interface is more important than a clever implementation. :-) > > If you'd like to keep the loop, then avoiding the bitfield is fine. In either > case, I think the struct should be passed by value rather than by reference. The > compiler is potentially smart enough to pack the four bools into a 32-bit int. FWIW, I think the loop is actually more readable than duplicating the same 5-6 lines of code 4+ times. The loop approach says "here's what I'm doing" from "this data" instead of having to make sure that the duplicated code has its own variable names (it could re-use but then you lose the DCHECKs) and properly replaces all bits of the duplicated code to point to its own things. I value compact/readable code more than saving a few bits (especially if the compiler is likely to optimize it out anyways).
All done. Committing. gab@: You're right, this code doesn't work correctly in a component build (all options are disabled in DLLs that haven't called InitializePreReadOptions). https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... File chrome/app/main_dll_loader_win.cc (right): https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { On 2016/01/21 19:49:27, gab wrote: > On 2016/01/21 18:15:42, grt wrote: > > On 2016/01/21 16:04:16, fdoray wrote: > > > On 2016/01/21 04:24:00, grt wrote: > > > > i think this would be easier to read if pre_read_option was actually > > something > > > > like: > > > > struct PreReadOptions { > > > > bool no_pre_read : 1; > > > > bool high_priority : 1; > > > > ... > > > > }; > > > > wdyt? > > > > > > Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't be > > > possible to take the address of members like I'm doing now in > > > InitializePreReadOptions [1]. If you really want me to use :1, I could > change > > > InitializePreReadOptions so that it doesn't need to take the address of > > members. > > > > > > [1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx > > > > I find the data-driven loop in InitializePreReadOptions interesting, but not > so > > compelling that it should dictate the overall interface. Four calls to ReadDW > > (or a tiny helper function) is quite clear, whereas the loop takes a little > > reading to validate. I'm okay with it as-is, just pointing out that a clean > > interface is more important than a clever implementation. :-) > > > > If you'd like to keep the loop, then avoiding the bitfield is fine. In either > > case, I think the struct should be passed by value rather than by reference. > The > > compiler is potentially smart enough to pack the four bools into a 32-bit int. > > FWIW, I think the loop is actually more readable than duplicating the same 5-6 > lines of code 4+ times. The loop approach says "here's what I'm doing" from > "this data" instead of having to make sure that the duplicated code has its own > variable names (it could re-use but then you lose the DCHECKs) and properly > replaces all bits of the duplicated code to point to its own things. > > I value compact/readable code more than saving a few bits (especially if the > compiler is likely to optimize it out anyways). I kept the loop, as preferred by gab@.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1610733002/#ps120001 (title: "don't return a const ref")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1610733002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1610733002/120001
On 2016/01/21 19:49:27, gab wrote: > lgtm > > Too bad that we can't DCHECK for initialization. I guess this also means that > this code doesn't work in component builds? (that's a pain for testing isn't > it?) > > https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... > File chrome/app/main_dll_loader_win.cc (right): > > https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... > chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & > startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { > On 2016/01/21 18:15:42, grt wrote: > > On 2016/01/21 16:04:16, fdoray wrote: > > > On 2016/01/21 04:24:00, grt wrote: > > > > i think this would be easier to read if pre_read_option was actually > > something > > > > like: > > > > struct PreReadOptions { > > > > bool no_pre_read : 1; > > > > bool high_priority : 1; > > > > ... > > > > }; > > > > wdyt? > > > > > > Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't be > > > possible to take the address of members like I'm doing now in > > > InitializePreReadOptions [1]. If you really want me to use :1, I could > change > > > InitializePreReadOptions so that it doesn't need to take the address of > > members. > > > > > > [1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx > > > > I find the data-driven loop in InitializePreReadOptions interesting, but not > so > > compelling that it should dictate the overall interface. Four calls to ReadDW > > (or a tiny helper function) is quite clear, whereas the loop takes a little > > reading to validate. I'm okay with it as-is, just pointing out that a clean > > interface is more important than a clever implementation. :-) > > > > If you'd like to keep the loop, then avoiding the bitfield is fine. In either > > case, I think the struct should be passed by value rather than by reference. > The > > compiler is potentially smart enough to pack the four bools into a 32-bit int. > > FWIW, I think the loop is actually more readable than duplicating the same 5-6 > lines of code 4+ times. I strongly disagree. The following is less code and, imo, easier to read: bool ReadBool(const base::win::RegKey& key, const base::char16* name) { DWORD value = 0; return key.ReadValueDW(name, &value) == ERROR_SUCCESS && value != 0; } void InitializePreReadOptions(const base::string16& product_registry_path) { ... option.no_pre_read = ReadBool(key, kNoPreReadVariationName); option.high_priority = ReadBool(key, kHighPriorityVariationName); option.only_if_cold = ReadBool(key, kOnlyIfColdVariationName); option.prefetch_virtual_memory = ReadBool(key, kPrefetchVirtualMemoryVariationName); ... > The loop approach says "here's what I'm doing" from > "this data" instead of having to make sure that the duplicated code has its own > variable names (it could re-use but then you lose the DCHECKs) and properly > replaces all bits of the duplicated code to point to its own things. It also hid a subtle bug that you missed in your review. :-) > I value compact/readable code more than saving a few bits (especially if the > compiler is likely to optimize it out anyways). I agree that saving bits isn't a priority, but a clean interface is.
Message was sent while issue was closed.
Description was changed from ========== Store PreRead options obtained from the registry in a global variable. Instead of reading PreRead options from the registry every time they are needed, read them once and store them in a global variable. This optimization isn't immediately useful as PreRead options are currently read only once per process. However, with the upcoming /prefetch:# experiment, the browser process will need the PreRead options every time it launches a process. This CL also has these benefits: - The caller of GetPreReadOptions doesn't need to know the product registry path. This will allow content to get the PreRead options to decide whether it should use a /prefetch:# switch when launching a process. - The PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ========== to ========== Store PreRead options obtained from the registry in a global variable. Instead of reading PreRead options from the registry every time they are needed, read them once and store them in a global variable. This optimization isn't immediately useful as PreRead options are currently read only once per process. However, with the upcoming /prefetch:# experiment, the browser process will need the PreRead options every time it launches a process. This CL also has these benefits: - The caller of GetPreReadOptions doesn't need to know the product registry path. This will allow content to get the PreRead options to decide whether it should use a /prefetch:# switch when launching a process. - The PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Store PreRead options obtained from the registry in a global variable. Instead of reading PreRead options from the registry every time they are needed, read them once and store them in a global variable. This optimization isn't immediately useful as PreRead options are currently read only once per process. However, with the upcoming /prefetch:# experiment, the browser process will need the PreRead options every time it launches a process. This CL also has these benefits: - The caller of GetPreReadOptions doesn't need to know the product registry path. This will allow content to get the PreRead options to decide whether it should use a /prefetch:# switch when launching a process. - The PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 ========== to ========== Store PreRead options obtained from the registry in a global variable. Instead of reading PreRead options from the registry every time they are needed, read them once and store them in a global variable. This optimization isn't immediately useful as PreRead options are currently read only once per process. However, with the upcoming /prefetch:# experiment, the browser process will need the PreRead options every time it launches a process. This CL also has these benefits: - The caller of GetPreReadOptions doesn't need to know the product registry path. This will allow content to get the PreRead options to decide whether it should use a /prefetch:# switch when launching a process. - The PreRead options are returned as a bit field instead of by setting multiple booleans. This will facilitate the addition of new PreRead options. BUG=577698 Committed: https://crrev.com/dee064299d3b3a13d5cc35f219d87d6a8478f227 Cr-Commit-Position: refs/heads/master@{#370805} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dee064299d3b3a13d5cc35f219d87d6a8478f227 Cr-Commit-Position: refs/heads/master@{#370805}
Message was sent while issue was closed.
On 2016/01/21 21:24:12, grt wrote: > On 2016/01/21 19:49:27, gab wrote: > > lgtm > > > > Too bad that we can't DCHECK for initialization. I guess this also means that > > this code doesn't work in component builds? (that's a pain for testing isn't > > it?) > > > > > https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... > > File chrome/app/main_dll_loader_win.cc (right): > > > > > https://codereview.chromium.org/1610733002/diff/60001/chrome/app/main_dll_loa... > > chrome/app/main_dll_loader_win.cc:72: if (!(pre_read_options & > > startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) { > > On 2016/01/21 18:15:42, grt wrote: > > > On 2016/01/21 16:04:16, fdoray wrote: > > > > On 2016/01/21 04:24:00, grt wrote: > > > > > i think this would be easier to read if pre_read_option was actually > > > something > > > > > like: > > > > > struct PreReadOptions { > > > > > bool no_pre_read : 1; > > > > > bool high_priority : 1; > > > > > ... > > > > > }; > > > > > wdyt? > > > > > > > > Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't > be > > > > possible to take the address of members like I'm doing now in > > > > InitializePreReadOptions [1]. If you really want me to use :1, I could > > change > > > > InitializePreReadOptions so that it doesn't need to take the address of > > > members. > > > > > > > > [1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx > > > > > > I find the data-driven loop in InitializePreReadOptions interesting, but not > > so > > > compelling that it should dictate the overall interface. Four calls to > ReadDW > > > (or a tiny helper function) is quite clear, whereas the loop takes a little > > > reading to validate. I'm okay with it as-is, just pointing out that a clean > > > interface is more important than a clever implementation. :-) > > > > > > If you'd like to keep the loop, then avoiding the bitfield is fine. In > either > > > case, I think the struct should be passed by value rather than by reference. > > The > > > compiler is potentially smart enough to pack the four bools into a 32-bit > int. > > > > FWIW, I think the loop is actually more readable than duplicating the same 5-6 > > lines of code 4+ times. > > I strongly disagree. The following is less code and, imo, easier to read: > > bool ReadBool(const base::win::RegKey& key, const base::char16* name) { > DWORD value = 0; > return key.ReadValueDW(name, &value) == ERROR_SUCCESS && value != 0; > } > > void InitializePreReadOptions(const base::string16& product_registry_path) { > ... > option.no_pre_read = ReadBool(key, kNoPreReadVariationName); > option.high_priority = ReadBool(key, kHighPriorityVariationName); > option.only_if_cold = ReadBool(key, kOnlyIfColdVariationName); > option.prefetch_virtual_memory = ReadBool(key, > kPrefetchVirtualMemoryVariationName); > ... I like that. I guess the loop is trying to keep the body of this helper in the method but it's fine to take it out. I'm happy either way with slight preference for your suggestion now that I see it. > > > > The loop approach says "here's what I'm doing" from > > "this data" instead of having to make sure that the duplicated code has its > own > > variable names (it could re-use but then you lose the DCHECKs) and properly > > replaces all bits of the duplicated code to point to its own things. > > It also hid a subtle bug that you missed in your review. :-) You mean |= instead of &= bug? Good catch but that bug would have been just as likely by using a helper method (provided we still used bitfields; of course no bit fields => no bit manipulation bug regardless of loop or not :-)). > > > I value compact/readable code more than saving a few bits (especially if the > > compiler is likely to optimize it out anyways). > > I agree that saving bits isn't a priority, but a clean interface is. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
