Tweak commit interval for ScheduleSavePersistentPrefs.

Tweak commit interval for ScheduleSavePersistentPrefs.

For release builds this interval is decreased to 2 seconds.  Current 10 seconds seem to be too high
because user is able to do a lot of interactions in this period possibly leading to crash.
For debug builds this interval is instead increased to 100 seconds in order to make it easier
to trigger bugs.

BUG=chromium-os:13102
TEST=Manual

[Denis Lagno, 2011-03-18 15:29:01]

please take a look

[Paweł Hajdan Jr., 2011-03-18 19:59:03]

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:24: #ifdef NDEBUG
I'm not a fan of different behavior in debug vs. release builds. Could you
remove this #if?

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:25: const int kDefaultCommitIntervalMs =
2000;
I'm not sure whether decreasing this interval is the right solution to the
problem. I think we should be either committing everything immediately, or keep
the existing interval.

Another idea would be to make ScheduleWrite call DoScheduledWrite immediately.

Note: even if we post the write task with no delay, it's still executed on
separate thread. If the browser is going to be killed, that doesn't really
"solve" the "data is not saved" problem.

[brettw, 2011-03-18 20:30:21]

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:25: const int kDefaultCommitIntervalMs =
2000;
If we do keep this different, there should be a comment explaining why.

[Denis Lagno, 2011-03-21 01:18:53]

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:24: #ifdef NDEBUG
On 2011/03/18 19:59:03, Paweł Hajdan Jr. wrote:
> I'm not a fan of different behavior in debug vs. release builds. Could you
> remove this #if?

I removed dependency on NDEBUG but instead added dependency on OFFICIAL_BUILD or
GOOGLE_CHROME_BUILD

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:25: const int kDefaultCommitIntervalMs =
2000;
On 2011/03/18 20:30:21, brettw wrote:
> If we do keep this different, there should be a comment explaining why.

Done.

http://codereview.chromium.org/6713033/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:25: const int kDefaultCommitIntervalMs =
2000;
On 2011/03/18 19:59:03, Paweł Hajdan Jr. wrote:
> I'm not sure whether decreasing this interval is the right solution to the
> problem. I think we should be either committing everything immediately, or
keep
> the existing interval.
> 
> Another idea would be to make ScheduleWrite call DoScheduledWrite immediately.
> 
> Note: even if we post the write task with no delay, it's still executed on
> separate thread. If the browser is going to be killed, that doesn't really
> "solve" the "data is not saved" problem.

original problem I've encountered was the fact that chromeos::RestartJob call
fails to commit pending writes for PrefService.  This problem was resolved by
enforcing committing on this exit path.

I like current implementation with delayed writes.  We keep a lot of stuff in
PrefServices and I am quite sure that there are bursts of writes to them so
commit interval is good optimization.

However I believe that optimal value of commit interval for stable version of
Chrome does not exceed 1-2 seconds.  Values higher than 1-2 seconds give
near-zero gain in performance but they give increasing unsafety.  Probability of
occurence of some unrelated crash grows fast with time passed.  User can do a
real lot in 10 seconds:)

[brettw, 2011-03-21 05:59:31]

http://codereview.chromium.org/6713033/diff/4003/chrome/common/important_file...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713033/diff/4003/chrome/common/important_file...
chrome/common/important_file_writer.cc:24: #if defined(OFFICIAL_BUILD) ||
defined(GOOGLE_CHROME_BUILD)
This seems like a much much worse idea than NDEBUG vs DEBUG. The point is that
the different behavior makes it hard to find bugs. Now we have something that
real users will run but almost no developer runs in their development
environment.

[Denis Lagno, 2011-03-21 13:07:21]

http://codereview.chromium.org/6713033/diff/4003/chrome/common/important_file...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713033/diff/4003/chrome/common/important_file...
chrome/common/important_file_writer.cc:24: #if defined(OFFICIAL_BUILD) ||
defined(GOOGLE_CHROME_BUILD)
On 2011/03/21 05:59:31, brettw wrote:
> This seems like a much much worse idea than NDEBUG vs DEBUG. The point is that
> the different behavior makes it hard to find bugs. Now we have something that
> real users will run but almost no developer runs in their development
> environment.

another attempt..

[Paweł Hajdan Jr., 2011-03-21 13:27:43]

http://codereview.chromium.org/6713033/diff/5004/chrome/common/important_file...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/6713033/diff/5004/chrome/common/important_file...
chrome/common/important_file_writer.cc:39: if (base::RandInt(0, 1))
I'm strongly against any randomness in this code. We have tools to detect
problems, and randomness in the code itself just makes reproducing problems more
complicated. Ditto about the #defines in the area.

On the other hand, decreasing the commit interval to 2s is fine for me.