Log pref file size histogram on read rather than on write.

Log pref file size histogram on read rather than on write.

All we care about is the size of the file per user, logging it on write
was wrong as it would generate WAY too many reports as indicated by
issue 453454.

Also changed the distribution to be logarithmic rather than linear to
give better insights (with a linear distribution all we could see was that
most people were below 209KB but we didn't have finer buckets below that).

BUG=453454, 355722
Committed: https://crrev.com/f16b59a08d0a0d8587cd968084a75f6e30799787
Cr-Commit-Position: refs/heads/master@{#314064}

[gab, 2015-01-30 13:25:26]

gab@chromium.org changed reviewers:
+ asvitkine@chromium.org

[gab, 2015-01-30 13:25:27]

Alexei PTAL.

Thanks for reporting this issue!
Gab

https://codereview.chromium.org/891663003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/891663003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:32557: +  </obsolete>
@asvitkine: it seemed more appropriate to deprecate the old histogram and create
a new one (if anything this may even help you clean up the logs cluster?), or do
you think we should just re-use the old one given it's really the same value
just reported differently?

[Alexei Svitkine (slow), 2015-01-30 13:49:03]

Thanks for cleaning this up! I agree the read histogram should be more useful
and should have fewer samples as well (reduce log use).

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
File base/json/json_file_value_serializer.cc (right):

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.cc:70: "Settings.JsonDataSizeKilobytes." +
histogram_suffix_,
Should be JsonDataReadSizeKilobytes.

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.h:18: :
JSONFileValueSerializer(json_file_path, std::string()) {}
Nit: Indent 2 more. Maybe run git cl format?

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.h:31: histogram_suffix_(histogram_suffix)
{}
Nit: The ctors and dtor should probably be defined out of line.

https://codereview.chromium.org/891663003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/891663003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:61880: -  <affected-histogram
name="Settings.JsonDataSizeKilobytes"/>
Keep this here, but mark the other histogram as <obsolete>.

[gab, 2015-01-30 17:33:37]

Thanks, PTAL.

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
File base/json/json_file_value_serializer.cc (right):

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.cc:70: "Settings.JsonDataSizeKilobytes." +
histogram_suffix_,
On 2015/01/30 13:49:03, Alexei Svitkine wrote:
> Should be JsonDataReadSizeKilobytes.

Oops, done.

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.h:18: :
JSONFileValueSerializer(json_file_path, std::string()) {}
On 2015/01/30 13:49:03, Alexei Svitkine wrote:
> Nit: Indent 2 more. Maybe run git cl format?

Good point, git cl formatted.

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.h:31: histogram_suffix_(histogram_suffix)
{}
On 2015/01/30 13:49:03, Alexei Svitkine wrote:
> Nit: The ctors and dtor should probably be defined out of line.

They were already inlined so I'm not sure this CL justifies changing that, I
think the reason clang allows this is that all members are POD-types (or made
only of POD types) -- i.e., all construction/destruction can be inlined.

https://codereview.chromium.org/891663003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/891663003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:61880: -  <affected-histogram
name="Settings.JsonDataSizeKilobytes"/>
On 2015/01/30 13:49:03, Alexei Svitkine wrote:
> Keep this here, but mark the other histogram as <obsolete>.

Done.

[Alexei Svitkine (slow), 2015-01-30 17:41:16]

lgtm

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.h:31: histogram_suffix_(histogram_suffix)
{}
On 2015/01/30 17:33:37, gab wrote:
> On 2015/01/30 13:49:03, Alexei Svitkine wrote:
> > Nit: The ctors and dtor should probably be defined out of line.
> 
> They were already inlined so I'm not sure this CL justifies changing that, I
> think the reason clang allows this is that all members are POD-types (or made
> only of POD types) -- i.e., all construction/destruction can be inlined.

But they're not POD types... a std::string is backed by a buffer on the heap and
involves a string copy...

I think the motivation for the rule is that we don't want ctors/dtors that
generate a lot of machine code (e.g. in this case, the ctors for std::string can
be inlined in this one) to be in headers, as then they end up in multiple object
files and have to be de-duped by the linker, slowing the build. I think the
above still applies to this case.

Also, since you're adding a new ctor, this change is making this existing
problem worse. So I would prefer you do actually out of line them... but won't
block you on this if you really insist...

[gab, 2015-01-30 18:22:34]

gab@chromium.org changed reviewers:
+ thakis@chromium.org

[gab, 2015-01-30 18:22:35]

Thanks, @Nico as base/json owner.

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/1/base/json/json_file_value_se...
base/json/json_file_value_serializer.h:31: histogram_suffix_(histogram_suffix)
{}
On 2015/01/30 17:41:16, Alexei Svitkine wrote:
> On 2015/01/30 17:33:37, gab wrote:
> > On 2015/01/30 13:49:03, Alexei Svitkine wrote:
> > > Nit: The ctors and dtor should probably be defined out of line.
> > 
> > They were already inlined so I'm not sure this CL justifies changing that, I
> > think the reason clang allows this is that all members are POD-types (or
made
> > only of POD types) -- i.e., all construction/destruction can be inlined.
> 
> But they're not POD types... a std::string is backed by a buffer on the heap
and
> involves a string copy...
> 
> I think the motivation for the rule is that we don't want ctors/dtors that
> generate a lot of machine code (e.g. in this case, the ctors for std::string
can
> be inlined in this one) to be in headers, as then they end up in multiple
object
> files and have to be de-duped by the linker, slowing the build. I think the
> above still applies to this case.
> 
> Also, since you're adding a new ctor, this change is making this existing
> problem worse. So I would prefer you do actually out of line them... but won't
> block you on this if you really insist...

Okay you convinced me, done.

[Nico, 2015-01-30 18:46:28]

https://codereview.chromium.org/891663003/diff/40001/base/json/json_file_valu...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/40001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:25: const std::string& histogram_suffix);
Why does a json library need to expose histograms in its public api?

Even the previous histogram code in the json implementation looks pretty fishy
to me (I hadn't seen it before), but at least it was some internal thing.

[Alexei Svitkine (slow), 2015-01-30 19:06:34]

Good point, Nico!

Maybe we could have the class have a getter to return how much size was read?
Then, it could be logged in the caller instead of being passed in to the ctor?

[gab, 2015-01-30 19:19:38]

On 2015/01/30 19:06:34, Alexei Svitkine wrote:
> Good point, Nico!
> 
> Maybe we could have the class have a getter to return how much size was read?
> Then, it could be logged in the caller instead of being passed in to the ctor?

Exposing an

int get_last_read_size() { return last_read_size_; }

method on JsonFileValueSerializer is an option. I was trying to avoid
delegates/callbacks, but a simple getter could be nice and keep the histogram
logic in the impl of JsonPrefStore (I also agree that it's somewhat weird to
have it there too, I guess we could add another getter on it for the proper
users to be logging their own histograms, but it just felt easier to put it in
there as the histograms are then all generated from the same place with the same
logic).

[Nico, 2015-01-30 19:34:00]

(the above sounds good to me, fwiw)

On Fri, Jan 30, 2015 at 11:19 AM, <gab@chromium.org> wrote:

> On 2015/01/30 19:06:34, Alexei Svitkine wrote:
>
>> Good point, Nico!
>>
>
>  Maybe we could have the class have a getter to return how much size was
>> read?
>> Then, it could be logged in the caller instead of being passed in to the
>> ctor?
>>
>
> Exposing an
>
> int get_last_read_size() { return last_read_size_; }
>
> method on JsonFileValueSerializer is an option. I was trying to avoid
> delegates/callbacks, but a simple getter could be nice and keep the
> histogram
> logic in the impl of JsonPrefStore (I also agree that it's somewhat weird
> to
> have it there too, I guess we could add another getter on it for the proper
> users to be logging their own histograms, but it just felt easier to put
> it in
> there as the histograms are then all generated from the same place with
> the same
> logic).
>
> https://codereview.chromium.org/891663003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2015-01-30 19:55:00]

get_last_read_size() sgtm too

On Fri, Jan 30, 2015 at 2:33 PM, Nico Weber <thakis@chromium.org> wrote:

> (the above sounds good to me, fwiw)
>
> On Fri, Jan 30, 2015 at 11:19 AM, <gab@chromium.org> wrote:
>
>> On 2015/01/30 19:06:34, Alexei Svitkine wrote:
>>
>>> Good point, Nico!
>>>
>>
>>  Maybe we could have the class have a getter to return how much size was
>>> read?
>>> Then, it could be logged in the caller instead of being passed in to the
>>> ctor?
>>>
>>
>> Exposing an
>>
>> int get_last_read_size() { return last_read_size_; }
>>
>> method on JsonFileValueSerializer is an option. I was trying to avoid
>> delegates/callbacks, but a simple getter could be nice and keep the
>> histogram
>> logic in the impl of JsonPrefStore (I also agree that it's somewhat weird
>> to
>> have it there too, I guess we could add another getter on it for the
>> proper
>> users to be logging their own histograms, but it just felt easier to put
>> it in
>> there as the histograms are then all generated from the same place with
>> the same
>> logic).
>>
>> https://codereview.chromium.org/891663003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2015-01-30 20:40:36]

Patchset #4 (id:60001) has been deleted

[gab, 2015-01-30 20:41:17]

Modified as discussed, PTAL.

Thanks,
Gab

[Alexei Svitkine (slow), 2015-01-30 20:46:32]

https://codereview.chromium.org/891663003/diff/80001/base/json/json_file_valu...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/80001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:73: // |Deserialize()| call.
Looks like you're only updating the field if ReadFileToString() is successful.
Please update the comment to mention this.

Also, maybe only log the histogram on success too..

https://codereview.chromium.org/891663003/diff/80001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:74: size_t get_last_read_size() { return
last_read_size_; }
Nit: const

[Nico, 2015-01-30 20:55:03]

base/json lgtm

[gab, 2015-01-31 16:46:56]

Thanks, done.

https://codereview.chromium.org/891663003/diff/80001/base/json/json_file_valu...
File base/json/json_file_value_serializer.h (right):

https://codereview.chromium.org/891663003/diff/80001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:73: // |Deserialize()| call.
On 2015/01/30 20:46:32, Alexei Svitkine wrote:
> Looks like you're only updating the field if ReadFileToString() is successful.
> Please update the comment to mention this.
> 
> Also, maybe only log the histogram on success too..

Done.

https://codereview.chromium.org/891663003/diff/80001/base/json/json_file_valu...
base/json/json_file_value_serializer.h:74: size_t get_last_read_size() { return
last_read_size_; }
On 2015/01/30 20:46:32, Alexei Svitkine wrote:
> Nit: const

Done.

[gab, 2015-01-31 16:47:38]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-01-31 16:47:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891663003/100001

[commit-bot: I haz the power, 2015-01-31 19:09:33]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-01-31 19:10:27]

Patchset 5 (id:??) landed as
https://crrev.com/f16b59a08d0a0d8587cd968084a75f6e30799787
Cr-Commit-Position: refs/heads/master@{#314064}