Tweak DOMStorageArea rate limit

Tweak DOMStorageArea rate limit

Increase the data rate and the commit rate.

BUG=480228
Committed: https://crrev.com/588427cc4c76ed7ef48af26df8718469d1169d4b
Cr-Commit-Position: refs/heads/master@{#330278}

[hashimoto, 2015-05-13 07:07:58]

Patchset #1 (id:1) has been deleted

[hashimoto, 2015-05-13 07:11:16]

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

[hashimoto, 2015-05-13 07:11:18]

I still think we should remove rate limiters, but increasing the rate limit
might be a good start to address the issue.

[michaeln, 2015-05-14 01:33:49]

https://codereview.chromium.org/1136123006/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1136123006/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:36: const int
kCommitDefaultDelaySecs = 10;
Please don't hastily remove the commit rate limiter. You asked about Local State
and Preference files compared to DomStorage areas, one  difference is the number
of files involved. There can be a large number of areas constantly running
commit timers.

https://codereview.chromium.org/1136123006/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:40: const int kMaxBytesPerMinute
= kPerStorageAreaQuota;
Adjusting the values is ok, but this adjustment goes too far.

This translates to 14Gbytes per day, 7 bad behaving apps could make for a
terabyte per week. Actually more because the limits are applied to the size of
the values provided, but due to sqlite journaling the amt of writes to disk is
about dbl.

[hashimoto, 2015-05-14 06:02:59]

IIUC these limiters were introduced to address a P2 issue crbug.com/176727,
without any metric to measure the effectiveness while risking users' data.
One thing I don't understand about crbug.com/176727 is what is supposed to be
achieved by reducing IO.
Is it to make SSD live long? To improve UI response? To reduce noisy sound from
HDD? Or, to satisfy a few verbose users?
Because there is no clear objective, there is no metric we can use as a
reference.

Compared to the original state (kCommitDefaultDelaySecs = 5 w/o rate limiters),
even with this patch you can still reduce the number of disk write by a factor
of 2+.

IMO 10MB/minute (=170KB/s) disk write is better than losing last 12 hours.
If there is a bad-behaving app (i.e. pages or extensions), it's the app's fault.
There is nothing Chrome can do to compensate apps' misbehaving without causing
serious adverse effects everywhere else.
It should be addressed by asking the app's developer (e.g. filing bugs like
b/18904726), or adding debugging instruments (e.g. nice UI to let users know who
is performing heavy IO, UMA histogram to measure the relationship between
LocalStorage IO and UI response).

[michaeln, 2015-05-15 00:18:57]

On 2015/05/14 06:02:59, hashimoto wrote:
> IIUC these limiters were introduced to address a P2 issue crbug.com/176727,
> without any metric to measure the effectiveness while risking users' data.
> One thing I don't understand about crbug.com/176727 is what is supposed to be
> achieved by reducing IO.
> Is it to make SSD live long? To improve UI response? To reduce noisy sound
from
> HDD? Or, to satisfy a few verbose users?
> Because there is no clear objective, there is no metric we can use as a
> reference.

The bug about excessive IO is a legitimate complaint. The limiter was put in
place to help mitigate that. It is effective at reducing disk IO. Its too
aggressive about the limits and no attempt was made to flush in the shutdown
sequences that kill the process prior to a clean exit.

wdyt about these numbers?
const int kMaxBytesPerHour = kPerStorageAreaQuota;  // longest delay is an hour
const int kMaxCommitsPerHour = 60;  // typical delay less than a minute

> Compared to the original state (kCommitDefaultDelaySecs = 5 w/o rate
limiters),
> even with this patch you can still reduce the number of disk write by a factor
> of 2+.
>
> IMO 10MB/minute (=170KB/s) disk write is better than losing last 12 hours.

Lets pick a better balance.

> If there is a bad-behaving app (i.e. pages or extensions), it's the app's
fault.
> There is nothing Chrome can do to compensate apps' misbehaving without causing
> serious adverse effects everywhere else.
> It should be addressed by asking the app's developer (e.g. filing bugs like
> b/18904726), or adding debugging instruments (e.g. nice UI to let users know
who
> is performing heavy IO, UMA histogram to measure the relationship between
> LocalStorage IO and UI response).

The numbers mentioned in b/18904726 were surprising large. That a single site
running in the background, out of site and mind, without a user looking at it,
could incur that much IO is pretty bad. It doesn't scale to file bugs against
sites, there is a recurring pattern of 'write something on a timer', from the
point of view of a single app its not too  bad (so bugs filed don't get much
attn), but in aggregate it gets unacceptably bad.

[hashimoto, 2015-05-15 06:25:56]

On 2015/05/15 00:18:57, michaeln wrote:
> On 2015/05/14 06:02:59, hashimoto wrote:
> > IIUC these limiters were introduced to address a P2 issue crbug.com/176727,
> > without any metric to measure the effectiveness while risking users' data.
> > One thing I don't understand about crbug.com/176727 is what is supposed to
be
> > achieved by reducing IO.
> > Is it to make SSD live long? To improve UI response? To reduce noisy sound
> from
> > HDD? Or, to satisfy a few verbose users?
> > Because there is no clear objective, there is no metric we can use as a
> > reference.
> 
> The bug about excessive IO is a legitimate complaint. The limiter was put in
The main problem here is that there is no clear goal.
"Let's reduce disk IO" is not a goal, it's rather a slogan.
Since there is no clear goal, you cannot even say if these rate limits are
sufficient or not.
If there is a clear goal (e.g. reducing UI jank), we can set up needed
instruments, define the target value (e.g. don't block the UI for more than N
ms) and land needed changes, and will be able to declare a win eventually.
> place to help mitigate that. It is effective at reducing disk IO. Its too
> aggressive about the limits and no attempt was made to flush in the shutdown
> sequences that kill the process prior to a clean exit.
> 
> wdyt about these numbers?
> const int kMaxBytesPerHour = kPerStorageAreaQuota;  // longest delay is an
hour
> const int kMaxCommitsPerHour = 60;  // typical delay less than a minute
> 
> > Compared to the original state (kCommitDefaultDelaySecs = 5 w/o rate
> limiters),
> > even with this patch you can still reduce the number of disk write by a
factor
> > of 2+.
> >
> > IMO 10MB/minute (=170KB/s) disk write is better than losing last 12 hours.
> 
> Lets pick a better balance.
How can you say this is a better balance when there is no clear goal nor metric
which we can refer to?
> 
> > If there is a bad-behaving app (i.e. pages or extensions), it's the app's
> fault.
> > There is nothing Chrome can do to compensate apps' misbehaving without
causing
> > serious adverse effects everywhere else.
> > It should be addressed by asking the app's developer (e.g. filing bugs like
> > b/18904726), or adding debugging instruments (e.g. nice UI to let users know
> who
> > is performing heavy IO, UMA histogram to measure the relationship between
> > LocalStorage IO and UI response).
> 
> The numbers mentioned in b/18904726 were surprising large. That a single site
> running in the background, out of site and mind, without a user looking at it,
> could incur that much IO is pretty bad. It doesn't scale to file bugs against
> sites, there is a recurring pattern of 'write something on a timer', from the
> point of view of a single app its not too  bad (so bugs filed don't get much
> attn), but in aggregate it gets unacceptably bad.

When a native app is doing wrong, users solve it by closing/uninstalling it.
The same applies for sites, misbehaving sites should be closed (or apps be
uninstalled).

Even without rate limiters, there are many things we can do against this
problem:
- Filing bugs against sites/extensions.
- Adding UI to indicate which site is performing heavy IO (like the speaker icon
on tabs playing audio)
- Adding UMA to investigate what's happening on users' devices.
These things can be done without causing any serious adverse effects.
OTOH rate limiters are destined to risk users' data.

[hashimoto, 2015-05-15 06:29:26]

That being said, the suggested values look much better than the current values.
It might be good as a first step.

Made a new patch.
PTAL

[michaeln, 2015-05-15 20:25:01]

please update the cl description, lgtm

[hashimoto, 2015-05-16 03:16:46]

On 2015/05/15 20:25:01, michaeln wrote:
> please update the cl description, lgtm

Done.

[hashimoto, 2015-05-16 03:16:53]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2015-05-16 03:17:15]

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

[commit-bot: I haz the power, 2015-05-16 03:20:37]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-05-18 11:31:28]

Patchset 2 (id:??) landed as
https://crrev.com/588427cc4c76ed7ef48af26df8718469d1169d4b
Cr-Commit-Position: refs/heads/master@{#330278}