Adjust LocalStorage commit rate limiting.

Alter the numbers to max out at a one hour delay but typically run at once per minute. Explicitly flush LocalStorage when a browser session is ending and when all documents in an origin have been closed.

BUG=485304

[michaeln, 2015-05-14 03:33:44]

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

[michaeln, 2015-05-14 03:33:44]

wdyt?

[hashimoto, 2015-05-14 04:46:09]

https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:495: profile,
base::Bind(FlushStoragePartition, rundown_counter));
I'm not sure if this will work as expected.
Currently, rundown_counter is responsible to wait for base::ImportantFileWriter
to flush Preferences and Local State.
These tasks are already time consuming. (cf. base::ImportantFileWriter related
items on Profiler page of uma.googleplex.com)

Since it's very likely that almost all DOMStorageArea are having uncommitted
data with the current 12-hour commit interval, it may frequently fail to finish
all tasks before rundown_counter aborts.

Please note that the browser process is explicitly killed in SessionEnding()
after EndSession() returns.
This means if you fail to finish tasks within 10 seconds (=
kEndSessionTimeoutSeconds), data will be lost.

FWIW, I guess here the number of uncommitted data is more important than the
amount of uncommitted data.
Writing DB results in flushing files (e.g. calling fdatasync() on Linux) and it
can take seconds, depending on how busy the disk is.
Since the disk is expected to be extremely busy during OS shutdown, it'll be
more likely to hit the 10 second timeout.

IMO the most straightforward (and the only feasible) solution is removing rate
limiters from DOMStorageArea.

[michaeln, 2015-05-15 03:14:33]

https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:7: #include <algorithm>
On 2015/05/14 04:46:09, hashimoto wrote:
>
https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
> chrome/browser/browser_process_impl.cc:495: profile,
> base::Bind(FlushStoragePartition, rundown_counter));
> I'm not sure if this will work as expected.
> Currently, rundown_counter is responsible to wait for
base::ImportantFileWriter
> to flush Preferences and Local State.
> These tasks are already time consuming. (cf. base::ImportantFileWriter related
> items on Profiler page of http://uma.googleplex.com)
> 
> Since it's very likely that almost all DOMStorageArea are having uncommitted
> data with the current 12-hour commit interval, it may frequently fail to
finish
> all tasks before rundown_counter aborts.

I don't buy that almost all areas have uncommitted data. This would be a good
uma stat to gather.

With the existing limits, the avg commit delay time is about 3 mins (no where
near 12 hours). The second part of the change in this CL flushes areas upon the
closing of all pages for a particular origin which should help somewhat.

> Please note that the browser process is explicitly killed in SessionEnding()
> after EndSession() returns.
> This means if you fail to finish tasks within 10 seconds (=
> kEndSessionTimeoutSeconds), data will be lost.

Yes, that's why i'm keen to wait for domstorage commit tasks here too.
EndSession waits at most 10 seconds, if the couple tasks its currently waiting
on complete more quickly, it can terminate much sooner and then not even try to
store the domstorage data.

> FWIW, I guess here the number of uncommitted data is more important than the
> amount of uncommitted data.
> Writing DB results in flushing files (e.g. calling fdatasync() on Linux) and
it
> can take seconds, depending on how busy the disk is.
> Since the disk is expected to be extremely busy during OS shutdown, it'll be
> more likely to hit the 10 second timeout.

Another good stat would be how long it takes to flush domstorage?

With the current impl where the backing store for localstorage is a multiple
sqlitedbs (one per origin), the number of areas to flush may very well be more
significant than the amt of data. Might be more reason to switch to a single
leveldb, to batch flushed changes into a single commit?

> IMO the most straightforward (and the only feasible) solution is removing rate
> limiters from DOMStorageArea.

I'd vote to plug new numbers into the rate limiter as a first step.

const int kMaxBytesPerHour = kPerStorageAreaQuota;  // longest delay is an hour
const int kMaxCommitsPerHour = 60;  // typical delay less than a minute

[hashimoto, 2015-05-15 06:59:10]

Changes proposed in this CL looks good to me except the EndSession() part.
I don't think it's meaningful to put this code in EndSession(), but I'd defer it
to a legitimate owner of browser_process_impl.cc.

Please note that there are many other services which need to persist data on the
disk (e.g. HistoryService), but they are doing nothing in EndSession().
Obviously, it's a bad thing to do to put everything in EndSession() (the same
goes for FlushPersistentData() in c/b/android/chromium_application.cc).
Preferences and Local State are covered by EndSession() because they are very
important, but I'm not sure if LocalStorage is of the same importance as them.

On 2015/05/15 03:14:33, michaeln wrote:
>
https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
> chrome/browser/browser_process_impl.cc:7: #include <algorithm>
> On 2015/05/14 04:46:09, hashimoto wrote:
> >
>
https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
> > File chrome/browser/browser_process_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1140013003/diff/20001/chrome/browser/browser_...
> > chrome/browser/browser_process_impl.cc:495: profile,
> > base::Bind(FlushStoragePartition, rundown_counter));
> > I'm not sure if this will work as expected.
> > Currently, rundown_counter is responsible to wait for
> base::ImportantFileWriter
> > to flush Preferences and Local State.
> > These tasks are already time consuming. (cf. base::ImportantFileWriter
related
> > items on Profiler page of http://uma.googleplex.com)
> > 
> > Since it's very likely that almost all DOMStorageArea are having uncommitted
> > data with the current 12-hour commit interval, it may frequently fail to
> finish
> > all tasks before rundown_counter aborts.
> 
> I don't buy that almost all areas have uncommitted data. This would be a good
> uma stat to gather.
> 
> With the existing limits, the avg commit delay time is about 3 mins (no where
ComputeCommitDelay() always returns 5 seconds for the first call, and it starts
returning values larger than 10 minutes after that.
When thinking about shutdown, you should exclude these samples.
After excluding the samples whose value is 5 seconds, the average becomes 10+
minutes, unsurprisingly.
> near 12 hours). The second part of the change in this CL flushes areas upon
the
> closing of all pages for a particular origin which should help somewhat.
> 
> > Please note that the browser process is explicitly killed in SessionEnding()
> > after EndSession() returns.
> > This means if you fail to finish tasks within 10 seconds (=
> > kEndSessionTimeoutSeconds), data will be lost.
> 
> Yes, that's why i'm keen to wait for domstorage commit tasks here too.
> EndSession waits at most 10 seconds, if the couple tasks its currently waiting
> on complete more quickly, it can terminate much sooner and then not even try
to
> store the domstorage data.
> 
> > FWIW, I guess here the number of uncommitted data is more important than the
> > amount of uncommitted data.
> > Writing DB results in flushing files (e.g. calling fdatasync() on Linux) and
> it
> > can take seconds, depending on how busy the disk is.
> > Since the disk is expected to be extremely busy during OS shutdown, it'll be
> > more likely to hit the 10 second timeout.
> 
> Another good stat would be how long it takes to flush domstorage?
> 
> With the current impl where the backing store for localstorage is a multiple
> sqlitedbs (one per origin), the number of areas to flush may very well be more
> significant than the amt of data. Might be more reason to switch to a single
> leveldb, to batch flushed changes into a single commit?
> 
> > IMO the most straightforward (and the only feasible) solution is removing
rate
> > limiters from DOMStorageArea.
> 
> I'd vote to plug new numbers into the rate limiter as a first step.
> 
> const int kMaxBytesPerHour = kPerStorageAreaQuota;  // longest delay is an
hour
> const int kMaxCommitsPerHour = 60;  // typical delay less than a minute