DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab the values from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org

Committed: https://crrev.com/8f224809c2603590ed31259f3d2f0e69494ee417
Cr-Commit-Position: refs/heads/master@{#416652}

[jsbell, 2016-09-02 19:20:25]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 19:20:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-09-02 19:21:46]

Description was changed from

==========
DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab them from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org
==========

to

==========
DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab the values from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org
==========

[jsbell, 2016-09-02 19:22:10]

michaeln@ - please take a look?

[commit-bot: I haz the power, 2016-09-02 21:53:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 21:53:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[michaeln, 2016-09-02 22:39:57]

lgtm almost

https://codereview.chromium.org/2309523002/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/2309523002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_area.cc:199:
commit_batch->changed_values[key] = base::NullableString16(value, false);
looks like you probably meant to put a non-null "" empty string value or a null
NullableString16() here

https://codereview.chromium.org/2309523002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_area.cc:337: if (commit_batch_)
Juggling where to set map_ null is a little awkward. Not sure this suggestion is
any better but I think you could assert(backing_) if commit_batch_ is non null
and put the test up before the early return for no backing. Nothing can be
committed without a backing store.

if (commit_batch_) {
  DCHECK(backing_);
  PopulateCommitBatchValues();
}

map_ = NULL;
if (!backing_)
  return;

[michaeln, 2016-09-02 22:46:30]

Something like this makes sense for LevelDBWrapperImpl too. At some point, that
class will replace DOMStorageArea for localstore usage.

[jsbell, 2016-09-02 22:52:30]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 22:52:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-09-02 22:54:20]

https://codereview.chromium.org/2309523002/diff/1/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/2309523002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_area.cc:199:
commit_batch->changed_values[key] = base::NullableString16(value, false);
On 2016/09/02 22:39:57, michaeln wrote:
> looks like you probably meant to put a non-null "" empty string value or a
null
> NullableString16() here

Oh, duh. Lost that at some point. :P

Done.

https://codereview.chromium.org/2309523002/diff/1/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_area.cc:337: if (commit_batch_)
On 2016/09/02 22:39:57, michaeln wrote:
> Juggling where to set map_ null is a little awkward. Not sure this suggestion
is
> any better but I think you could assert(backing_) if commit_batch_ is non null
> and put the test up before the early return for no backing. Nothing can be
> committed without a backing store.
> 
> if (commit_batch_) {
>   DCHECK(backing_);
>   PopulateCommitBatchValues();
> }
> 
> map_ = NULL;
> if (!backing_)
>   return;

Yeah, I went through several iterations here. I do slightly prefer yours, so
going with that.

[jsbell, 2016-09-02 23:08:41]

On 2016/09/02 22:46:30, michaeln wrote:
> Something like this makes sense for LevelDBWrapperImpl too. At some point,
that
> class will replace DOMStorageArea for localstore usage.

This CL or a follow-up? Do you have a tracking bug for that work?

[michaeln, 2016-09-02 23:30:27]

On 2016/09/02 23:08:41, jsbell wrote:
> On 2016/09/02 22:46:30, michaeln wrote:
> > Something like this makes sense for LevelDBWrapperImpl too. At some point,
> that
> > class will replace DOMStorageArea for localstore usage.
> 
> This CL or a follow-up? Do you have a tracking bug for that work?

I'd vote for a follow-up, so if something goes wrong with one of the two impls,
it doesn't cause the other to be reverted. There is a tracking bug for the
mojo-ifing of domstorage, crbug/586194.

[commit-bot: I haz the power, 2016-09-02 23:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 23:46:10]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-09-06 16:30:28]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-09-06 16:30:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2309523002/#ps20001
(title: "Actually save the memory")

[commit-bot: I haz the power, 2016-09-06 16:30:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 17:45:38]

Description was changed from

==========
DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab the values from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org
==========

to

==========
DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab the values from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org
==========

[commit-bot: I haz the power, 2016-09-06 17:45:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-06 17:48:11]

Description was changed from

==========
DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab the values from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org
==========

to

==========
DOM Storage: Populate commit batch values lazily to avoid memory bloat

DOM Storage (a.k.a. localStorage and sessionStorage) flushes changes
to disks on a timer. It previously held updated keys/values in both an
in-memory map and a "commit batch" of pending updates to write. For
large values (which are common) this duplicated memory until the timer
fired.

To avoid this, keep only the keys in the commit batch until just
before we toss the batch over to the commit thread, and at that point
grab the values from the in-memory map.

BUG=640698
R=michaeln@chromium.org,dskiba@chromium.org

Committed: https://crrev.com/8f224809c2603590ed31259f3d2f0e69494ee417
Cr-Commit-Position: refs/heads/master@{#416652}
==========

[commit-bot: I haz the power, 2016-09-06 17:48:12]

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