Stop pretty-printing pref JSON

Stop pretty-printing pref JSON

To reduce the amount of data written out to the disk during shutdown to avoid slowing it down.
In practice, this results in about 30% size reduction of Preferences file.

BUG=463366
Committed: https://crrev.com/6fe43614243c9e8935a01be3fb149ccc2d8d4c4e
Cr-Commit-Position: refs/heads/master@{#319211}

[hashimoto, 2015-03-03 05:08:42]

hashimoto@chromium.org changed reviewers:
+ bauerb@chromium.org

[hashimoto, 2015-03-03 05:08:42]

Hi Bernhard,

I'd like to remove this line to make the serialized preference data 30% smaller
if it's OK.
Is there any reason to make the data human-readable?

[Bernhard Bauer, 2015-03-03 09:17:00]

On 2015/03/03 05:08:42, hashimoto wrote:
> Hi Bernhard,
> 
> I'd like to remove this line to make the serialized preference data 30%
smaller
> if it's OK.
> Is there any reason to make the data human-readable?

The major reason for making prefs human-readable is to facilitate debugging or
changing invidividual preferences. I still think those are good use cases, but
if there are major performance improvements to be made by pretty-printing, they
probably don't outweigh those (and one can still pretty-print manually to
debug).

TL;DR: LGTM

[hashimoto, 2015-03-03 09:47:48]

On 2015/03/03 09:17:00, Bernhard Bauer wrote:
> On 2015/03/03 05:08:42, hashimoto wrote:
> > Hi Bernhard,
> > 
> > I'd like to remove this line to make the serialized preference data 30%
> smaller
> > if it's OK.
> > Is there any reason to make the data human-readable?
> 
> The major reason for making prefs human-readable is to facilitate debugging or
> changing invidividual preferences. I still think those are good use cases, but
> if there are major performance improvements to be made by pretty-printing,
they
> probably don't outweigh those (and one can still pretty-print manually to
> debug).
> 
> TL;DR: LGTM

Thanks,
This should be helpful to reduce Chrome OS crash reports as Chrome gets killed
on Chrome OS when it cannot finish shutdown within 3 seconds. (crbug.com/418627)
Probably it might be useful to add a commandline switch to do pretty-printing
for developers.

[hashimoto, 2015-03-03 09:58:42]

hashimoto@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[hashimoto, 2015-03-03 09:59:05]

TBRing phajdan.jr@chromium.org for base/test/data/prefs/write.golden.json

[Bernhard Bauer, 2015-03-03 10:53:02]

On 2015/03/03 09:47:48, hashimoto wrote:
> On 2015/03/03 09:17:00, Bernhard Bauer wrote:
> > On 2015/03/03 05:08:42, hashimoto wrote:
> > > Hi Bernhard,
> > > 
> > > I'd like to remove this line to make the serialized preference data 30%
> > smaller
> > > if it's OK.
> > > Is there any reason to make the data human-readable?
> > 
> > The major reason for making prefs human-readable is to facilitate debugging
or
> > changing invidividual preferences. I still think those are good use cases,
but
> > if there are major performance improvements to be made by pretty-printing,
> they
> > probably don't outweigh those (and one can still pretty-print manually to
> > debug).
> > 
> > TL;DR: LGTM
> 
> Thanks,
> This should be helpful to reduce Chrome OS crash reports as Chrome gets killed
> on Chrome OS when it cannot finish shutdown within 3 seconds.
(crbug.com/418627)
> Probably it might be useful to add a commandline switch to do pretty-printing
> for developers.

Yeah, that's a good idea!

[Paweł Hajdan Jr., 2015-03-03 11:30:23]

base/test/data LGTM

[Bernhard Bauer, 2015-03-03 12:38:02]

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

[Bernhard Bauer, 2015-03-03 12:38:03]

+gab

[gab, 2015-03-03 13:15:43]

Just saw this, not lgtm just yet, as I stressed on the bug a few minutes ago: I
think we need a flag (command-line and potentially even chrome://flags) to
enable pretty-printing. This is required IMO to make some testing/debugging
scenarios less painful.

Thanks!
Gab

[gab, 2015-03-03 13:16:50]

On 2015/03/03 13:15:43, gab wrote:
> Just saw this, not lgtm just yet, as I stressed on the bug a few minutes ago:
I
> think we need a flag (command-line and potentially even chrome://flags) to
> enable pretty-printing. This is required IMO to make some testing/debugging
> scenarios less painful.
> 
> Thanks!
> Gab

Please also justify this change by highlighting in the CL description how much
this saves in practice (e.g. ~33% on typical preferences from my local tests).

[hashimoto, 2015-03-03 13:44:51]

On 2015/03/03 13:16:50, gab wrote:
> On 2015/03/03 13:15:43, gab wrote:
> > Just saw this, not lgtm just yet, as I stressed on the bug a few minutes
ago:
> I
> > think we need a flag (command-line and potentially even chrome://flags) to
> > enable pretty-printing. This is required IMO to make some testing/debugging
> > scenarios less painful.
> > 
> > Thanks!
> > Gab
> 
> Please also justify this change by highlighting in the CL description how much
> this saves in practice (e.g. ~33% on typical preferences from my local tests).

Updated the CL description and removed the TBR line.
Will work on the new command line switch tomorrow.

[gab, 2015-03-03 13:53:50]

On 2015/03/03 13:44:51, hashimoto wrote:
> On 2015/03/03 13:16:50, gab wrote:
> > On 2015/03/03 13:15:43, gab wrote:
> > > Just saw this, not lgtm just yet, as I stressed on the bug a few minutes
> ago:
> > I
> > > think we need a flag (command-line and potentially even chrome://flags) to
> > > enable pretty-printing. This is required IMO to make some
testing/debugging
> > > scenarios less painful.
> > > 
> > > Thanks!
> > > Gab
> > 
> > Please also justify this change by highlighting in the CL description how
much
> > this saves in practice (e.g. ~33% on typical preferences from my local
tests).
> 
> Updated the CL description and removed the TBR line.
> Will work on the new command line switch tomorrow.

Thanks, will promptly review upon update.

[hashimoto, 2015-03-04 05:37:30]

Uploaded a new patch set with "--pretty-print-prefs" switch.
PTAL

[Bernhard Bauer, 2015-03-04 10:07:58]

LGTM if Gab is happy.

https://codereview.chromium.org/974753002/diff/40001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/974753002/diff/40001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:404: switches::kPrettyPrintPrefs))
Add braces around the body if the condition is more complicated than a single
line, for readability.

[hashimoto, 2015-03-04 14:18:40]

https://codereview.chromium.org/974753002/diff/40001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/974753002/diff/40001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:404: switches::kPrettyPrintPrefs))
On 2015/03/04 10:07:58, Bernhard Bauer wrote:
> Add braces around the body if the condition is more complicated than a single
> line, for readability.

Done.

[gab, 2015-03-04 16:33:58]

lgtm w/ comment about export

Thanks,
Gab

https://codereview.chromium.org/974753002/diff/60001/base/prefs/base_prefs_sw...
File base/prefs/base_prefs_switches.h (right):

https://codereview.chromium.org/974753002/diff/60001/base/prefs/base_prefs_sw...
base/prefs/base_prefs_switches.h:12: BASE_PREFS_EXPORT extern const char
kPrettyPrintPrefs[];
Hmm, does this actually need to be exported? It's only used inside this module.

[hashimoto, 2015-03-04 17:05:49]

https://codereview.chromium.org/974753002/diff/60001/base/prefs/base_prefs_sw...
File base/prefs/base_prefs_switches.h (right):

https://codereview.chromium.org/974753002/diff/60001/base/prefs/base_prefs_sw...
base/prefs/base_prefs_switches.h:12: BASE_PREFS_EXPORT extern const char
kPrettyPrintPrefs[];
On 2015/03/04 16:33:57, gab wrote:
> Hmm, does this actually need to be exported? It's only used inside this
module.

I thought it might be nice to follow examples like net/base/net_export.h.
Stopped exporting it as there is no strong reason to do it.

[hashimoto, 2015-03-04 17:06:43]

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

[hashimoto, 2015-03-04 17:06:43]

thakis@, could you review this change as an owner of base/base.gyp and
base/BUILD.gn?

[Nico, 2015-03-04 17:43:21]

lgtm, cool!

[hashimoto, 2015-03-05 03:25:03]

The CQ bit was checked by hashimoto@chromium.org

[hashimoto, 2015-03-05 03:25:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, bauerb@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/974753002/#ps80001 (title:
"No export")

[commit-bot: I haz the power, 2015-03-05 03:26:22]

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

[commit-bot: I haz the power, 2015-03-05 03:57:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-05 03:58:10]

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