Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2010173005:
Support experiment to store persistent metrics in memory-mapped file. (Closed)
DescriptionSupport experiment to store persistent metrics in memory-mapped file.
Metrics have to be persisted to disk and one possibility
is to simply use file-backed memory which leaves the
responsibility to the OS. It is simple and reliable but
there are concerns about what impact this could have on
performance.
Enabling this as an experiment will allow direct
collection of real data as to how Chrome is affected
by it and thus make possible an informed decision about
whether to use it or not.
BUG=546019
Committed: https://crrev.com/2a27d6c635cedc801939765d271834b15ea4d299
Cr-Commit-Position: refs/heads/master@{#403368}
Patch Set 1 #Patch Set 2 : restored lost line #Patch Set 3 : omit file actions on NACL builds #Patch Set 4 : rebased #
Total comments: 7
Patch Set 5 : moved file cleanup to anonymous function #
Total comments: 16
Patch Set 6 : addressed (most) review comments by Ilya #Patch Set 7 : switch to variation parameter #Patch Set 8 : rebased #
Total comments: 8
Patch Set 9 : rebased #Patch Set 10 : addressed review comments by Alexei #
Total comments: 12
Patch Set 11 : some comment changes #
Messages
Total messages: 43 (11 generated)
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
Besides the comments below, please expand the CL description with more info - for example to answer the question "why?". Also please note that if you're doing work in chrome/ - it won't immediately apply to iOS. So if you want this to be active in iOS, you need to make the code be in a component and used from ios/ parallels of these classes. Note: The above iOS case might already apply to some of your existing code even before this CL. Please check. https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || feature_group == "Disabled") Please don't use == to compare groups. Best practice is to use StartsWith() or use variation params instead. Actually would recommend to use params here, since you already use a feature. So you can use GetVariationParamsByFeature(). https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:217: // If there is a global metrics file being updated on disk, mark it to be Please make a function in the anon namespace that does this and just call it from here. By the way, are you sure this object is guaranteed to be deleted?
Description was changed from ========== Support experiment to store persistent metrics in memory-mapped file. BUG=546019 ========== to ========== Support experiment to store persistent metrics in memory-mapped file. Metrics have to be persisted to disk and one possibility is to simply use file-backed memory which leaves the responsibility to the OS. It is simple and reliable but there are concerns about what impact this could have on performance. Enabling this as an experiment will allow direct collection of real data as to how Chrome is affected by it and thus make possible an informed decision about whether to use it or not. BUG=546019 ==========
https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || feature_group == "Disabled") On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > Please don't use == to compare groups. Best practice is to use StartsWith() or > use variation params instead. > > Actually would recommend to use params here, since you already use a feature. So > you can use GetVariationParamsByFeature(). I started down that path but found it more complicated and still required an arbitrary string comparison. BTW, is there a particular reason why the "name" isn't available from the Feature API? Is it worth adding? https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:217: // If there is a global metrics file being updated on disk, mark it to be On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > Please make a function in the anon namespace that does this and just call it > from here. > > By the way, are you sure this object is guaranteed to be deleted? Done. I checked that the object is deleted on clean shutdown. I don't know if it's guaranteed but it's not a problem if it is not and probably desired to keep it around it if anything abnormal happens.
https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || feature_group == "Disabled") On 2016/06/16 14:21:08, bcwhite wrote: > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > Please don't use == to compare groups. Best practice is to use StartsWith() or > > use variation params instead. > > > > Actually would recommend to use params here, since you already use a feature. > So > > you can use GetVariationParamsByFeature(). > > I started down that path but found it more complicated and still required an > arbitrary string comparison. String =='s is fine for variations params, since that still allows you to have differently named groups set the same params. > > BTW, is there a particular reason why the "name" isn't available from the > Feature API? Is it worth adding? > Do you mean field trial name? Because you're using name here. :)
https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || feature_group == "Disabled") On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > On 2016/06/16 14:21:08, bcwhite wrote: > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > Please don't use == to compare groups. Best practice is to use StartsWith() > or > > > use variation params instead. > > > > > > Actually would recommend to use params here, since you already use a > feature. > > So > > > you can use GetVariationParamsByFeature(). > > > > I started down that path but found it more complicated and still required an > > arbitrary string comparison. > > String =='s is fine for variations params, since that still allows you to have > differently named groups set the same params. > > > > > BTW, is there a particular reason why the "name" isn't available from the > > Feature API? Is it worth adding? > > > > Do you mean field trial name? Because you're using name here. :) I meant the experiment name as in "Enabled", "Disable", "MemoryMappedFile", etc.
https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || feature_group == "Disabled") On 2016/06/16 14:31:43, bcwhite wrote: > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > On 2016/06/16 14:21:08, bcwhite wrote: > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > Please don't use == to compare groups. Best practice is to use > StartsWith() > > or > > > > use variation params instead. > > > > > > > > Actually would recommend to use params here, since you already use a > > feature. > > > So > > > > you can use GetVariationParamsByFeature(). > > > > > > I started down that path but found it more complicated and still required an > > > arbitrary string comparison. > > > > String =='s is fine for variations params, since that still allows you to have > > differently named groups set the same params. > > > > > > > > BTW, is there a particular reason why the "name" isn't available from the > > > Feature API? Is it worth adding? > > > > > > > Do you mean field trial name? Because you're using name here. :) > > I meant the experiment name as in "Enabled", "Disable", "MemoryMappedFile", etc. Because we're discouraging using experiment name to make decisions, in favor of params. :) (Because it's error prone if you use == and then you realise you have data bias because you rolled your Enabled group to 100% on dogfood and 50/50 for everyone else and then your Enabled vs. Disabled comparison is completely biased on platforms when dogfooders are a large chunk of the Enabled users... If you use params, then you can use different group names and avoid this problem.)
On 2016/06/16 14:34:40, Alexei Svitkine (OOO) wrote: > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_field_trials.cc (right): > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || > feature_group == "Disabled") > On 2016/06/16 14:31:43, bcwhite wrote: > > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > > On 2016/06/16 14:21:08, bcwhite wrote: > > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > > Please don't use == to compare groups. Best practice is to use > > StartsWith() > > > or > > > > > use variation params instead. > > > > > > > > > > Actually would recommend to use params here, since you already use a > > > feature. > > > > So > > > > > you can use GetVariationParamsByFeature(). > > > > > > > > I started down that path but found it more complicated and still required > an > > > > arbitrary string comparison. > > > > > > String =='s is fine for variations params, since that still allows you to > have > > > differently named groups set the same params. > > > > > > > > > > > BTW, is there a particular reason why the "name" isn't available from the > > > > Feature API? Is it worth adding? > > > > > > > > > > Do you mean field trial name? Because you're using name here. :) > > > > I meant the experiment name as in "Enabled", "Disable", "MemoryMappedFile", > etc. > > Because we're discouraging using experiment name to make decisions, in favor of > params. :) > > (Because it's error prone if you use == and then you realise you have data bias > because you rolled your Enabled group to 100% on dogfood and 50/50 for everyone > else and then your Enabled vs. Disabled comparison is completely biased on > platforms when dogfooders are a large chunk of the Enabled users... If you use > params, then you can use different group names and avoid this problem.) Let's go over this when we meet. I think it'll be less confusing. :-)
On 2016/06/16 14:34:40, Alexei Svitkine (OOO) wrote: > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_field_trials.cc (right): > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > chrome/browser/chrome_browser_field_trials.cc:63: if (feature_group.empty() || > feature_group == "Disabled") > On 2016/06/16 14:31:43, bcwhite wrote: > > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > > On 2016/06/16 14:21:08, bcwhite wrote: > > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > > Please don't use == to compare groups. Best practice is to use > > StartsWith() > > > or > > > > > use variation params instead. > > > > > > > > > > Actually would recommend to use params here, since you already use a > > > feature. > > > > So > > > > > you can use GetVariationParamsByFeature(). > > > > > > > > I started down that path but found it more complicated and still required > an > > > > arbitrary string comparison. > > > > > > String =='s is fine for variations params, since that still allows you to > have > > > differently named groups set the same params. > > > > > > > > > > > BTW, is there a particular reason why the "name" isn't available from the > > > > Feature API? Is it worth adding? > > > > > > > > > > Do you mean field trial name? Because you're using name here. :) > > > > I meant the experiment name as in "Enabled", "Disable", "MemoryMappedFile", > etc. > > Because we're discouraging using experiment name to make decisions, in favor of > params. :) > > (Because it's error prone if you use == and then you realise you have data bias > because you rolled your Enabled group to 100% on dogfood and 50/50 for everyone > else and then your Enabled vs. Disabled comparison is completely biased on > platforms when dogfooders are a large chunk of the Enabled users... If you use > params, then you can use different group names and avoid this problem.) Let's go over this when we meet. I think it'll be less confusing. :-)
I'm in Munich right and only working today. It's about 5PM now so I'm about to leave the office. So sorry, no meeting today. And I'm still on vacation next week... On Thu, Jun 16, 2016 at 4:42 PM, <bcwhite@chromium.org> wrote: > On 2016/06/16 14:34:40, Alexei Svitkine (OOO) wrote: > > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > > File chrome/browser/chrome_browser_field_trials.cc (right): > > > > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > > chrome/browser/chrome_browser_field_trials.cc:63: if > (feature_group.empty() || > > feature_group == "Disabled") > > On 2016/06/16 14:31:43, bcwhite wrote: > > > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > > > On 2016/06/16 14:21:08, bcwhite wrote: > > > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > > > Please don't use == to compare groups. Best practice is to use > > > StartsWith() > > > > or > > > > > > use variation params instead. > > > > > > > > > > > > Actually would recommend to use params here, since you already > use a > > > > feature. > > > > > So > > > > > > you can use GetVariationParamsByFeature(). > > > > > > > > > > I started down that path but found it more complicated and still > required > > an > > > > > arbitrary string comparison. > > > > > > > > String =='s is fine for variations params, since that still allows > you to > > have > > > > differently named groups set the same params. > > > > > > > > > > > > > > BTW, is there a particular reason why the "name" isn't available > from > the > > > > > Feature API? Is it worth adding? > > > > > > > > > > > > > Do you mean field trial name? Because you're using name here. :) > > > > > > I meant the experiment name as in "Enabled", "Disable", > "MemoryMappedFile", > > etc. > > > > Because we're discouraging using experiment name to make decisions, in > favor > of > > params. :) > > > > (Because it's error prone if you use == and then you realise you have > data > bias > > because you rolled your Enabled group to 100% on dogfood and 50/50 for > everyone > > else and then your Enabled vs. Disabled comparison is completely biased > on > > platforms when dogfooders are a large chunk of the Enabled users... If > you use > > params, then you can use different group names and avoid this problem.) > > Let's go over this when we meet. I think it'll be less confusing. :-) > > > https://codereview.chromium.org/2010173005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/16 14:44:46, Alexei Svitkine (OOO) wrote: > I'm in Munich right and only working today. It's about 5PM now so I'm about > to leave the office. So sorry, no meeting today. And I'm still on vacation > next week... > > On Thu, Jun 16, 2016 at 4:42 PM, <mailto:bcwhite@chromium.org> wrote: > > > On 2016/06/16 14:34:40, Alexei Svitkine (OOO) wrote: > > > > > > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > > > File chrome/browser/chrome_browser_field_trials.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > > > chrome/browser/chrome_browser_field_trials.cc:63: if > > (feature_group.empty() || > > > feature_group == "Disabled") > > > On 2016/06/16 14:31:43, bcwhite wrote: > > > > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > > > > On 2016/06/16 14:21:08, bcwhite wrote: > > > > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > > > > Please don't use == to compare groups. Best practice is to use > > > > StartsWith() > > > > > or > > > > > > > use variation params instead. > > > > > > > > > > > > > > Actually would recommend to use params here, since you already > > use a > > > > > feature. > > > > > > So > > > > > > > you can use GetVariationParamsByFeature(). > > > > > > > > > > > > I started down that path but found it more complicated and still > > required > > > an > > > > > > arbitrary string comparison. > > > > > > > > > > String =='s is fine for variations params, since that still allows > > you to > > > have > > > > > differently named groups set the same params. > > > > > > > > > > > > > > > > > BTW, is there a particular reason why the "name" isn't available > > from > > the > > > > > > Feature API? Is it worth adding? > > > > > > > > > > > > > > > > Do you mean field trial name? Because you're using name here. :) > > > > > > > > I meant the experiment name as in "Enabled", "Disable", > > "MemoryMappedFile", > > > etc. > > > > > > Because we're discouraging using experiment name to make decisions, in > > favor > > of > > > params. :) > > > > > > (Because it's error prone if you use == and then you realise you have > > data > > bias > > > because you rolled your Enabled group to 100% on dogfood and 50/50 for > > everyone > > > else and then your Enabled vs. Disabled comparison is completely biased > > on > > > platforms when dogfooders are a large chunk of the Enabled users... If > > you use > > > params, then you can use different group names and avoid this problem.) > > > > Let's go over this when we meet. I think it'll be less confusing. :-) > > > > > > https://codereview.chromium.org/2010173005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Oh, okay. And I'll be on vacation for two weeks starting on the 27th. Is it important to change this?
Yes, I don't think the CL should be submitted as-is. Shall we wait until you're back? On Thu, Jun 16, 2016 at 4:46 PM, <bcwhite@chromium.org> wrote: > On 2016/06/16 14:44:46, Alexei Svitkine (OOO) wrote: > > I'm in Munich right and only working today. It's about 5PM now so I'm > about > > to leave the office. So sorry, no meeting today. And I'm still on > vacation > > next week... > > > > On Thu, Jun 16, 2016 at 4:42 PM, <mailto:bcwhite@chromium.org> wrote: > > > > > On 2016/06/16 14:34:40, Alexei Svitkine (OOO) wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > > > > File chrome/browser/chrome_browser_field_trials.cc (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > > > > chrome/browser/chrome_browser_field_trials.cc:63: if > > > (feature_group.empty() || > > > > feature_group == "Disabled") > > > > On 2016/06/16 14:31:43, bcwhite wrote: > > > > > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > > > > > On 2016/06/16 14:21:08, bcwhite wrote: > > > > > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > > > > > Please don't use == to compare groups. Best practice is to > use > > > > > StartsWith() > > > > > > or > > > > > > > > use variation params instead. > > > > > > > > > > > > > > > > Actually would recommend to use params here, since you > already > > > use a > > > > > > feature. > > > > > > > So > > > > > > > > you can use GetVariationParamsByFeature(). > > > > > > > > > > > > > > I started down that path but found it more complicated and > still > > > required > > > > an > > > > > > > arbitrary string comparison. > > > > > > > > > > > > String =='s is fine for variations params, since that still > allows > > > you to > > > > have > > > > > > differently named groups set the same params. > > > > > > > > > > > > > > > > > > > > BTW, is there a particular reason why the "name" isn't > available > > > from > > > the > > > > > > > Feature API? Is it worth adding? > > > > > > > > > > > > > > > > > > > Do you mean field trial name? Because you're using name here. :) > > > > > > > > > > I meant the experiment name as in "Enabled", "Disable", > > > "MemoryMappedFile", > > > > etc. > > > > > > > > Because we're discouraging using experiment name to make decisions, > in > > > favor > > > of > > > > params. :) > > > > > > > > (Because it's error prone if you use == and then you realise you have > > > data > > > bias > > > > because you rolled your Enabled group to 100% on dogfood and 50/50 > for > > > everyone > > > > else and then your Enabled vs. Disabled comparison is completely > biased > > > on > > > > platforms when dogfooders are a large chunk of the Enabled users... > If > > > you use > > > > params, then you can use different group names and avoid this > problem.) > > > > > > Let's go over this when we meet. I think it'll be less confusing. :-) > > > > > > > > > https://codereview.chromium.org/2010173005/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Oh, okay. And I'll be on vacation for two weeks starting on the 27th. > > Is it important to change this? > > > https://codereview.chromium.org/2010173005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Yes, I don't think the CL should be submitted as-is. Shall we wait until > you're back? Sure.
If not, you can ask another reviewer to continue the review on my behalf. On Jun 16, 2016 4:34 PM, <asvitkine@chromium.org> wrote: > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_field_trials.cc (right): > > > https://codereview.chromium.org/2010173005/diff/100001/chrome/browser/chrome_... > chrome/browser/chrome_browser_field_trials.cc:63: if > (feature_group.empty() || feature_group == "Disabled") > On 2016/06/16 14:31:43, bcwhite wrote: > > On 2016/06/16 14:28:02, Alexei Svitkine (OOO) wrote: > > > On 2016/06/16 14:21:08, bcwhite wrote: > > > > On 2016/06/16 11:36:57, Alexei Svitkine (OOO) wrote: > > > > > Please don't use == to compare groups. Best practice is to use > > StartsWith() > > > or > > > > > use variation params instead. > > > > > > > > > > Actually would recommend to use params here, since you already > use a > > > feature. > > > > So > > > > > you can use GetVariationParamsByFeature(). > > > > > > > > I started down that path but found it more complicated and still > required an > > > > arbitrary string comparison. > > > > > > String =='s is fine for variations params, since that still allows > you to have > > > differently named groups set the same params. > > > > > > > > > > > BTW, is there a particular reason why the "name" isn't available > from the > > > > Feature API? Is it worth adding? > > > > > > > > > > Do you mean field trial name? Because you're using name here. :) > > > > I meant the experiment name as in "Enabled", "Disable", > "MemoryMappedFile", etc. > > Because we're discouraging using experiment name to make decisions, in > favor of params. :) > > (Because it's error prone if you use == and then you realise you have > data bias because you rolled your Enabled group to 100% on dogfood and > 50/50 for everyone else and then your Enabled vs. Disabled comparison is > completely biased on platforms when dogfooders are a large chunk of the > Enabled users... If you use params, then you can use different group > names and avoid this problem.) > > https://codereview.chromium.org/2010173005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
bcwhite@chromium.org changed reviewers: + isherman@chromium.org
+Ilya, would you mind continuing this? I'd like to get this in before the branch-point on June 30th.
Here's one round of comments, but I'll also be OOO next week -- sorry! https://codereview.chromium.org/2010173005/diff/120001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2010173005/diff/120001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:444: const FilePath& GetPersistentLocation(); nit: Can the method be marked as const as well? https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:37: base::kPersistentHistogramsFeature.name); nit: Please move this down to where it's used. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:41: NOTREACHED(); nit: Chrome style guidelines recommend either a NOTREACHED() or an early return, but not both. In this case, an early return is probably more appropriate, since the filesystem can get corrupted or fail in other ways. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: return; Please use the FeatureList API to test whether the feature is enabled, so that enabling/disabling via the command-line works. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:79: ChromeMetricsServiceClient::kBrowserMetricsName); I *think* the latest advice that I saw about using multiple experimental states in conjunction with the base::FeatureList API is to have a single Enabled group, and then to use field trial params to control the parameters of how the trial is enabled. Alexei knows this better than I do, though. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:83: return; nit: Ditto about NOTREACHED() or early return. In this case, I'd actually omit both -- there's no reason for this code path to be reachable. Or, if you're worried about corruption (?), then an early return seems like the right choice. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:185: void CleanupGlobalPersistentHistogramStorage() { nit: s/Cleanup/CleanUp https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:189: // during the next startup sequence. To clarify: Is there any harm in keeping the file? Would metrics be doubly-reported if it were to remain? Or is this purely an optimization? If it's purely an optimization, I wonder whether it's important enough to do -- it slows down shutdown a bit, and makes the code a bit harder to reason about. OTOH, if it's to avoid duplicated data, I worry that it might not be robust enough. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:196: base::File::FLAG_DELETE_ON_CLOSE); nit: This seems like a somewhat indirect way to delete a file. Is there not something more direct in our APIs?
https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:79: ChromeMetricsServiceClient::kBrowserMetricsName); On 2016/06/17 19:54:32, Ilya Sherman (Away June 18-26) wrote: > I *think* the latest advice that I saw about using multiple experimental states > in conjunction with the base::FeatureList API is to have a single Enabled group, > and then to use field trial params to control the parameters of how the trial is > enabled. Alexei knows this better than I do, though. Ah, I actually wrote this before going back and reading the comment history on the CL. Seems like Alexei said roughly the same thing =)
I honestly don't see the improvement in a Variation over the Group Name. It's especially horrible on the command line, requiring two settings to make anything happen. But it's done. https://codereview.chromium.org/2010173005/diff/120001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2010173005/diff/120001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:444: const FilePath& GetPersistentLocation(); On 2016/06/17 19:54:32, Ilya Sherman (Away June 18-26) wrote: > nit: Can the method be marked as const as well? Done. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:41: NOTREACHED(); On 2016/06/17 19:54:32, Ilya Sherman (Away June 18-26) wrote: > nit: Chrome style guidelines recommend either a NOTREACHED() or an early return, > but not both. In this case, an early return is probably more appropriate, since > the filesystem can get corrupted or fail in other ways. Done. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:79: ChromeMetricsServiceClient::kBrowserMetricsName); On 2016/06/17 19:57:23, Ilya Sherman (Away June 18-26) wrote: > On 2016/06/17 19:54:32, Ilya Sherman (Away June 18-26) wrote: > > I *think* the latest advice that I saw about using multiple experimental > states > > in conjunction with the base::FeatureList API is to have a single Enabled > group, > > and then to use field trial params to control the parameters of how the trial > is > > enabled. Alexei knows this better than I do, though. > > Ah, I actually wrote this before going back and reading the comment history on > the CL. Seems like Alexei said roughly the same thing =) Done. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:185: void CleanupGlobalPersistentHistogramStorage() { On 2016/06/17 19:54:32, Ilya Sherman (Away June 18-26) wrote: > nit: s/Cleanup/CleanUp Done. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:189: // during the next startup sequence. > To clarify: Is there any harm in keeping the file? Would metrics be > doubly-reported if it were to remain? Or is this purely an optimization? If > it's purely an optimization, I wonder whether it's important enough to do -- it > slows down shutdown a bit, and makes the code a bit harder to reason about. > OTOH, if it's to avoid duplicated data, I worry that it might not be robust > enough. It's very important so that startup doesn't process all the data in the file when there is nothing in it. Due to a metrics collection just previous in the shutdown process, everything should be zeros. Collection of the "previous run" information is done in the foreground during startup so gathering this data delays the appearance of the first browser window. Removing this file during clean exit means no penalty on the next startup. https://codereview.chromium.org/2010173005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:196: base::File::FLAG_DELETE_ON_CLOSE); On 2016/06/17 19:54:32, Ilya Sherman (Away June 18-26) wrote: > nit: This seems like a somewhat indirect way to delete a file. Is there not > something more direct in our APIs? Unfortunately no. base::DeleteFile() won't work on an open file on Windows.
bcwhite@chromium.org changed reviewers: + jar@chromium.org
+jar, Alexei and Ilya are away this week and I'm away for the two after that. I'd like to get this in before the Beta branch. I've addressed all their comments; can you check the Variation code and let me know if this is okay?
Ping?
https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:36: void InstantiatePersistentHistograms() { Both this code and corresponding code in chrome_metrics_service_client.cc is getting pretty involved now. I suggest moving things to a new file (e.g. persistent_metrics_initialization.cc) where both this code and CreateInstallerFileMetricsProvider() can live. This way, ChromeMetricsServiceClient::kBrowserMetricsName can be made local to that file and you can make a helper function for generating the metrics file name that you can share instead of duplicating code. I'm OK with doing all of the above in a separate follow-up CL - so you can just add a TODO here for now that you can address in a follow-up CL. Thanks! https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:53: // read when reporting initial stability metrics. If there is no file to I'm not sure this behavior makes sense. The initial stability metrics report (that uses the "saved" system profile from the previous session) will only report histograms that are marked as "uma stability histograms". So, assuming that file is opened and its contents are merged into the global statistics recorder, then what happens is that only stability metrics will be picked up and reported correctly (with the old system profile), but the other metrics won't be and will persist and get reported by this browser's next upload, which uses a system profile that doesn't correspond to last session. I don't think we want the above behavior since it will cause confusing metrics. https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: std::string storage = variations::GetVariationParamValue( Use GetVariationParamValueByFeature(). https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:192: if (allocator) { Nit: Use early returns instead.
Patchset #9 (id:200001) has been deleted
https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:36: void InstantiatePersistentHistograms() { On 2016/06/28 15:08:00, Alexei Svitkine (very slow) wrote: > Both this code and corresponding code in chrome_metrics_service_client.cc is > getting pretty involved now. > > I suggest moving things to a new file (e.g. > persistent_metrics_initialization.cc) where both this code and > CreateInstallerFileMetricsProvider() can live. This way, > ChromeMetricsServiceClient::kBrowserMetricsName can be made local to that file > and you can make a helper function for generating the metrics file name that you > can share instead of duplicating code. > > I'm OK with doing all of the above in a separate follow-up CL - so you can just > add a TODO here for now that you can address in a follow-up CL. Thanks! Acknowledged. https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:53: // read when reporting initial stability metrics. If there is no file to On 2016/06/28 15:08:00, Alexei Svitkine (very slow) wrote: > I'm not sure this behavior makes sense. > > The initial stability metrics report (that uses the "saved" system profile from > the previous session) will only report histograms that are marked as "uma > stability histograms". So, assuming that file is opened and its contents are > merged into the global statistics recorder, then what happens is that only > stability metrics will be picked up and reported correctly (with the old system > profile), but the other metrics won't be and will persist and get reported by > this browser's next upload, which uses a system profile that doesn't correspond > to last session. > > I don't think we want the above behavior since it will cause confusing metrics. The FileMetricsProvider, when doing RecordInitialHistogramSnapshots() that is called because it is tagged ASSOCIATE_PREVIOUS_RUN will send all non-zero metrics regardless of what flags it may have. None of the metrics from it will get merged into the SR. https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: std::string storage = variations::GetVariationParamValue( On 2016/06/28 15:08:00, Alexei Svitkine (very slow) wrote: > Use GetVariationParamValueByFeature(). Done. https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/180001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:192: if (allocator) { On 2016/06/28 15:08:00, Alexei Svitkine (very slow) wrote: > Nit: Use early returns instead. Done.
lgtm https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:197: // Open (with delete) and then immediately close the file by going out Nit: Wrapping is off? I think some words below should fit on this line.
https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: const size_t kAllocSize = 3 << 20; // 3 MiB Unrelated to this CL: What happens if the alloc size is exceeded, by the way? https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:187: // during the next startup sequence. nit: I'd move this comment to be just above the function, and also add a bit of text mentioning that this basically trades off slower shutdown vs. slower startup. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:199: // that may be open elsewhere. If the file is open elsewhere, is there anything that guarantees that there won't be any further attempts to read from or write to the file?
https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: const size_t kAllocSize = 3 << 20; // 3 MiB On 2016/06/29 20:41:38, Ilya Sherman wrote: > Unrelated to this CL: What happens if the alloc size is exceeded, by the way? In that case, new histograms will get allocated from the heap. Everything will run as normal except those on the heap will not be persisted. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:187: // during the next startup sequence. On 2016/06/29 20:41:38, Ilya Sherman wrote: > nit: I'd move this comment to be just above the function, and also add a bit of > text mentioning that this basically trades off slower shutdown vs. slower > startup. Done. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:197: // Open (with delete) and then immediately close the file by going out On 2016/06/29 19:13:43, Alexei Svitkine (very slow) wrote: > Nit: Wrapping is off? I think some words below should fit on this line. One word. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:199: // that may be open elsewhere. On 2016/06/29 20:41:38, Ilya Sherman wrote: > If the file is open elsewhere, is there anything that guarantees that there > won't be any further attempts to read from or write to the file? None. Existing opens will continue to operate normally. New opens will fail and the actual contents will be free'd once the last open handle gets closed.
LGTM. Thanks, Brian. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: const size_t kAllocSize = 3 << 20; // 3 MiB On 2016/06/30 22:56:12, bcwhite wrote: > On 2016/06/29 20:41:38, Ilya Sherman wrote: > > Unrelated to this CL: What happens if the alloc size is exceeded, by the way? > > In that case, new histograms will get allocated from the heap. Everything will > run as normal except those on the heap will not be persisted. Okay, sounds good. Do we have metrics in place to measure how frequently this occurs in practice? https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:199: // that may be open elsewhere. On 2016/06/30 22:56:12, bcwhite wrote: > On 2016/06/29 20:41:38, Ilya Sherman wrote: > > If the file is open elsewhere, is there anything that guarantees that there > > won't be any further attempts to read from or write to the file? > > None. Existing opens will continue to operate normally. New opens will fail > and the actual contents will be free'd once the last open handle gets closed. Ah, nice. I think this would be great to document explicitly in this comment -- that behavior wasn't obvious to me.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2010173005/#ps260001 (title: "some comment changes")
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 ========== Support experiment to store persistent metrics in memory-mapped file. Metrics have to be persisted to disk and one possibility is to simply use file-backed memory which leaves the responsibility to the OS. It is simple and reliable but there are concerns about what impact this could have on performance. Enabling this as an experiment will allow direct collection of real data as to how Chrome is affected by it and thus make possible an informed decision about whether to use it or not. BUG=546019 ========== to ========== Support experiment to store persistent metrics in memory-mapped file. Metrics have to be persisted to disk and one possibility is to simply use file-backed memory which leaves the responsibility to the OS. It is simple and reliable but there are concerns about what impact this could have on performance. Enabling this as an experiment will allow direct collection of real data as to how Chrome is affected by it and thus make possible an informed decision about whether to use it or not. BUG=546019 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Support experiment to store persistent metrics in memory-mapped file. Metrics have to be persisted to disk and one possibility is to simply use file-backed memory which leaves the responsibility to the OS. It is simple and reliable but there are concerns about what impact this could have on performance. Enabling this as an experiment will allow direct collection of real data as to how Chrome is affected by it and thus make possible an informed decision about whether to use it or not. BUG=546019 ========== to ========== Support experiment to store persistent metrics in memory-mapped file. Metrics have to be persisted to disk and one possibility is to simply use file-backed memory which leaves the responsibility to the OS. It is simple and reliable but there are concerns about what impact this could have on performance. Enabling this as an experiment will allow direct collection of real data as to how Chrome is affected by it and thus make possible an informed decision about whether to use it or not. BUG=546019 Committed: https://crrev.com/2a27d6c635cedc801939765d271834b15ea4d299 Cr-Commit-Position: refs/heads/master@{#403368} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2a27d6c635cedc801939765d271834b15ea4d299 Cr-Commit-Position: refs/heads/master@{#403368}
Message was sent while issue was closed.
On 2016/06/20 16:21:28, bcwhite wrote: > I honestly don't see the improvement in a Variation over the Group Name. It's > especially horrible on the command line, requiring two settings to make anything > happen. Circling back to this comment, I'd like to make the benefits easy to understand for future experimenters. Can you help me figure out what we need to improve in our docs for it? The main problem is that by expecting hard-coded group names, it makes it not possible to rename groups. And renaming groups is often needed when doing experimentation, for example: - If you launch at a different percent to Dogfood users (e.g. 100% to dogfood), then you want different group names, otherwise comparing Experiment vs. Control group will have a dogfood bias (which is significant for some users) - When introducing new groups, you want to launch a new control group, otherwise the old control will have more users in it (i.e. people who are on an older version of the experiment config) so comparing experiment vs. control group will be affected by the bias - Sometimes you may want to launch several parallel groups with the same behavior to confirm an effect (e.g. if you suspect there's some anomalous users in one of the groups causing an effect on metrics), again to do this we need the ability to rename groups. So the above are just some of the reasons. Hopefully, we can update our docs to make these clear to users such as yourself. I think right now we have some sporadic mentions of the issues on different pages at go/uma (e.g. on the dogfood page). Maybe we should have it somewhere in a more central place. Would that be helpful? For example, we can have a separate page for it and link it from the main experiment guide. What do you think?
Message was sent while issue was closed.
Sorry I couldn't get to this earlier. I was in Costa Rica with limited internet and a deadline. I'll update the comment in a new CL along with some other cleanup. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials.cc:64: const size_t kAllocSize = 3 << 20; // 3 MiB On 2016/06/30 23:16:48, Ilya Sherman wrote: > On 2016/06/30 22:56:12, bcwhite wrote: > > On 2016/06/29 20:41:38, Ilya Sherman wrote: > > > Unrelated to this CL: What happens if the alloc size is exceeded, by the > way? > > > > In that case, new histograms will get allocated from the heap. Everything > will > > run as normal except those on the heap will not be persisted. > > Okay, sounds good. Do we have metrics in place to measure how frequently this > occurs in practice? Not directly. There is a measure of how much of the memory segment is used. There is also a measure of how many histograms are allocated from each of heap and persistent memory. https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2010173005/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:199: // that may be open elsewhere. On 2016/06/30 23:16:48, Ilya Sherman wrote: > On 2016/06/30 22:56:12, bcwhite wrote: > > On 2016/06/29 20:41:38, Ilya Sherman wrote: > > > If the file is open elsewhere, is there anything that guarantees that there > > > won't be any further attempts to read from or write to the file? > > > > None. Existing opens will continue to operate normally. New opens will fail > > and the actual contents will be free'd once the last open handle gets closed. > > Ah, nice. I think this would be great to document explicitly in this comment -- > that behavior wasn't obvious to me. Will do. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
