New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 712405 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 586194
issue 730475



Sign in to add a comment

Integrate mojo localstorage implementation with Memory Dump infrastructure

Project Member Reported by mek@chromium.org, Apr 17 2017

Issue description

The existing localstorage implementation implements base::trace_event::MemoryDumpProvider to provide more detailed information on its memory usage in memory dumps. The new mojo/leveldb based localstorage implementation should likely do something similar.

Due to differences in architecture there will of course be differences in what can be reported (in particular mojo layer between localstorage and leveldb might make it hard to associate leveldb memory usage with localstorage the way the current implementation manages to associate sqlite memory with localstorage), but it probably still makes sense to try to provide some information.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6704b385bb406997f1e625c4fe89d788ded33ad6

commit 6704b385bb406997f1e625c4fe89d788ded33ad6
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Thu Jun 08 22:20:20 2017

Integrate new localstorage implementation with memory tracing.

This currently does not account for memory used by leveldb
itself, only for the data that is cached on top of that.

Bug:  712405 
Change-Id: Iab324f2a276cdc7fd8130e35d557d942c5e12a63
Reviewed-on: https://chromium-review.googlesource.com/527704
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: siddhartha sivakumar <ssid@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478102}
[modify] https://crrev.com/6704b385bb406997f1e625c4fe89d788ded33ad6/content/browser/dom_storage/dom_storage_context_wrapper.cc
[modify] https://crrev.com/6704b385bb406997f1e625c4fe89d788ded33ad6/content/browser/dom_storage/local_storage_context_mojo.cc
[modify] https://crrev.com/6704b385bb406997f1e625c4fe89d788ded33ad6/content/browser/dom_storage/local_storage_context_mojo.h
[modify] https://crrev.com/6704b385bb406997f1e625c4fe89d788ded33ad6/content/browser/dom_storage/local_storage_context_mojo_unittest.cc
[modify] https://crrev.com/6704b385bb406997f1e625c4fe89d788ded33ad6/content/browser/leveldb_wrapper_impl.cc
[modify] https://crrev.com/6704b385bb406997f1e625c4fe89d788ded33ad6/content/browser/leveldb_wrapper_impl.h

Comment 2 by mek@chromium.org, Jun 8 2017

Cc: ssid@chromium.org dskiba@chromium.org
So that CL does the first half of this, as discussed the second half (accounting for leveldb's memory usage, particularly in the context of localstorage) is a bit trickier due to the potential for the file service to live out-of-process.
I started attempting to implement that half in https://chromium-review.googlesource.com/c/528446/, but I only half understand what I'm doing, so I'm probably doing it wrong (although the output sort of seems to be okay?).

Comment 3 by ssid@chromium.org, Jun 12 2017

After adding the new provider, please update base/trace_event/memory_infra_background_whitelist.cc with the new dump names and provider name.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4

commit b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Tue Jun 13 22:47:09 2017

Account for leveldb memory usage in new localstorage implementation.

This is implemented by passing a MemoryAllocatorDumpGuid to the
leveldb service when opening a new database, which is then used
to correlate memory used by leveldb with the localstorage context
using said leveldb database.

Bug:  712405 
Change-Id: I0c1792c577b4abca77e28331c920d9e78fca53bc
Reviewed-on: https://chromium-review.googlesource.com/528446
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: siddhartha sivakumar <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479186}
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/leveldb_database_impl.cc
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/leveldb_database_impl.h
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/leveldb_service_impl.cc
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/leveldb_service_impl.h
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/leveldb_service_unittest.cc
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/public/interfaces/leveldb.mojom
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/components/leveldb/remote_iterator_unittest.cc
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/content/browser/dom_storage/local_storage_context_mojo.cc
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/content/browser/dom_storage/local_storage_context_mojo.h
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/mojo/common/BUILD.gn
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/mojo/common/common_custom_types_struct_traits.cc
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/mojo/common/common_custom_types_struct_traits.h
[add] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/mojo/common/memory_allocator_dump_cross_process_uid.mojom
[add] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/mojo/common/memory_allocator_dump_cross_process_uid.typemap
[modify] https://crrev.com/b94f83ceb5356f1c6c02c2cdc9b49e77d7dd10c4/mojo/common/typemaps.gni

Comment 5 by mek@chromium.org, Jun 15 2017

Blocking: 730475
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4337eacb0812850d54f10b90b134d6c08c1d6bd8

commit 4337eacb0812850d54f10b90b134d6c08c1d6bd8
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Jun 16 20:30:03 2017

Update memory infra background whitelist with new Localstorage dump names.

TBR=primiano@chromium.org

Bug:  712405 
Change-Id: I531ae0dbfd9d2ba0abfcc4e202f4cd7f612b40f4
Reviewed-on: https://chromium-review.googlesource.com/538813
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: siddhartha sivakumar <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480153}
[modify] https://crrev.com/4337eacb0812850d54f10b90b134d6c08c1d6bd8/base/trace_event/memory_infra_background_whitelist.cc

Comment 7 by mek@chromium.org, Jun 16 2017

Status: Fixed (was: Available)

Sign in to add a comment