[tracing] Add IndexDB database memory usages to tracing

[tracing] Add IndexDB database memory usages to tracing

Webpages using IndexDB could use a lot of memory in leveldb databases
(in order of Gbs crbug.com/533648). This Cl adds the memory usage of
the databases to tracing.

BUG=547066
Committed: https://crrev.com/8af3b11896d42eb8f032116b5f5fb237ab12305b
Cr-Commit-Position: refs/heads/master@{#378919}

[ssid, 2015-10-26 17:55:55]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2015-10-26 17:55:56]

This does not take care of threading yet. Once the issue in
https://codereview.chromium.org/1310513004/ is fixed, this will be updates.

[ssid, 2015-11-11 18:33:38]

The indexed_db is always accessed in BrowserMainLoop::indexed_db_thread. So, I
think it safe to have this dump provider.

[jsbell, 2015-11-11 18:38:33]

On 2015/11/11 18:33:38, ssid wrote:
> The indexed_db is always accessed in BrowserMainLoop::indexed_db_thread. So, I
> think it safe to have this dump provider.

We're considering moving IDB operations to a blocking sequenced worker pool -
will that impact how this needs to be done?

[ssid, 2015-11-11 18:54:05]

On 2015/11/11 18:38:33, jsbell wrote:
> On 2015/11/11 18:33:38, ssid wrote:
> > The indexed_db is always accessed in BrowserMainLoop::indexed_db_thread. So,
I
> > think it safe to have this dump provider.
> 
> We're considering moving IDB operations to a blocking sequenced worker pool -
> will that impact how this needs to be done?

Yes. Currently memory infra doesn't support sequenced task runner, but only
thread task runners. So this needs to be changed.
When is this change expected?

[jsbell, 2015-11-11 19:29:46]

On 2015/11/11 18:54:05, ssid wrote:
> On 2015/11/11 18:38:33, jsbell wrote:
> > On 2015/11/11 18:33:38, ssid wrote:
> > > The indexed_db is always accessed in BrowserMainLoop::indexed_db_thread.
So,
> I
> > > think it safe to have this dump provider.
> > 
> > We're considering moving IDB operations to a blocking sequenced worker pool
-
> > will that impact how this needs to be done?
> 
> Yes. Currently memory infra doesn't support sequenced task runner, but only
> thread task runners. So this needs to be changed.
> When is this change expected?

We don't have a timeframe - it's something we've wanted to do for years but
there were some blockers. https://crbug.com/552552 is tracking it.

[ssid, 2016-02-29 19:53:22]

Now that crrev.com/1618703004 looks stable, this can be reviewed. PTAL, thanks.

[jsbell, 2016-02-29 20:18:53]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2016-02-29 20:18:54]

https://codereview.chromium.org/1420193003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/1420193003/diff/60001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_database.cc:461:
pmd->CreateAllocatorDump("leveldb/index_db/" + file_name_for_tracing);
Sanity check: the filename here is going to be a serialization of the origin
(e.g. "http_localhost_4201.indexeddb.leveldb") and will not be globally unique
(since it's partitioned by user profile).

From this CL it looks like there would be duplicates/collisions if the user
visited the same site from two different profiles and then a dump was taken. Is
that problematic?

[ssid, 2016-02-29 23:23:30]

https://codereview.chromium.org/1420193003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/1420193003/diff/60001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_database.cc:461:
pmd->CreateAllocatorDump("leveldb/index_db/" + file_name_for_tracing);
On 2016/02/29 20:18:54, jsbell wrote:
> Sanity check: the filename here is going to be a serialization of the origin
> (e.g. "http_localhost_4201.indexeddb.leveldb") and will not be globally unique
> (since it's partitioned by user profile).
> 
> From this CL it looks like there would be duplicates/collisions if the user
> visited the same site from two different profiles and then a dump was taken.
Is
> that problematic?

True. I did not think about this case. made the string unique and added file
name as attribute.
Thanks.

[jsbell, 2016-02-29 23:39:08]

lgtm

[ssid, 2016-03-01 23:02:34]

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

[commit-bot: I haz the power, 2016-03-01 23:03:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420193003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420193003/120001

[commit-bot: I haz the power, 2016-03-02 00:48:36]

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

[commit-bot: I haz the power, 2016-03-02 00:48:37]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-03-02 06:21:55]

LGTM. Just one question: did you check in debug (or release that dcheck always
on) that the db is actually destroyed on the task_runner (so we don't hit the
unregistration dcheck)?

https://codereview.chromium.org/1420193003/diff/120001/content/browser/indexe...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/1420193003/diff/120001/content/browser/indexe...
content/browser/indexed_db/leveldb/leveldb_database.cc:461:
base::StringPrintf("leveldb/index_db/%p", db_.get()));
out of curiosity, why don't we name it just level_db/index_db/%s_%p % (filename,
ptr) ?

[ssid, 2016-03-02 23:17:50]

Yes checked with debug and the databases are destroyed in the same thread. It's
not sequenced task runner yet. So let's see how it changes.

https://codereview.chromium.org/1420193003/diff/120001/content/browser/indexe...
File content/browser/indexed_db/leveldb/leveldb_database.cc (right):

https://codereview.chromium.org/1420193003/diff/120001/content/browser/indexe...
content/browser/indexed_db/leveldb/leveldb_database.cc:461:
base::StringPrintf("leveldb/index_db/%p", db_.get()));
On 2016/03/02 06:21:55, Primiano (throttled til Mar 4) wrote:
> out of curiosity, why don't we name it just level_db/index_db/%s_%p %
(filename,
> ptr) ?

If file name is long, which is taken from websites names, then the number
actually goes far too the right in trace viewer. So thought no harm adding as
attribute.

[ssid, 2016-03-02 23:19:44]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-03-02 23:19:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1420193003/#ps120001
(title: "Try again?")

[commit-bot: I haz the power, 2016-03-02 23:20:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420193003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420193003/120001

[commit-bot: I haz the power, 2016-03-03 02:14:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-03 02:15:55]

Description was changed from

==========
[tracing] Add IndexDB database memory usages to tracing

Webpages using IndexDB could use a lot of memory in leveldb databases
(in order of Gbs crbug.com/533648). This Cl adds the memory usage of
the databases to tracing.

BUG=547066
==========

to

==========
[tracing] Add IndexDB database memory usages to tracing

Webpages using IndexDB could use a lot of memory in leveldb databases
(in order of Gbs crbug.com/533648). This Cl adds the memory usage of
the databases to tracing.

BUG=547066

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

[commit-bot: I haz the power, 2016-03-03 02:15:56]

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