kDefaultLogReuseOptionValue is true on Android, and caused huge spike in memory usage: see  issue 710909 .

It looks like when reuse_logs is true, .log file will grow up to ~4 MiB causing up to 4 MiB (write_buffer_size?) of memory usage after Open(). We can't afford 4 MiB on Android, because it's ~20% of the total memory Chrome allocates in the browser process.

write_buffer_size is the next thing. By default it's 4 MiB, but there is a function, WriteBufferSize() which sets it to something else depending on free disk space. However, only IndexedDB seems to call it. All other places that use LevelDB use the default.

See  crbug.com/710909#c25  and #26 for some reuse_logs-related experiments.

kDefaultLogReuseOptionValue can be changed on Android with little effect - it was only done for performance to reduce time to open the database. I am curious how this changed in M59 as manifest reuse was first enabled in M41.

One possibility (just a guess) is that the leveldb is failing to compact. Is it possible that the device is running out of file descriptors? See  issue 197371 . 

I think in that particular case (710909) the issue was that code was deleting 5760 keys on every Open(). I.e. that is way more than any other instance do. However, LevelDB is used in many places, so it's possible that other instances will exhibit the same problem, either by just accumulating entries in the log, or after a change that increases workload.

Regarding write_buffer_size: see issue 662019 where the buffer size was decreased to save memory.

I checked leveldb::Options in all places where leveldb::DB::Open() is called, and here are my observations:

* Most (but not all) places set reuse_logs to leveldb_env::kDefaultLogReuseOptionValue

* Many places set max_open_files to 0 (which is clamped by leveldb to 74), few set to 80. The prevailing comment when setting 0 is "Use minimum number of files".

* Many (but not all) set paranoid_checks = true.

* Few places set write_buffer_size: leveldb_env::WriteBufferSize() is used once, two other places set low value to save memory. Remember that the default is 4 MiB.

* Exactly one place sets block_cache to something smaller than the default 8 MiB.

* Exactly one place sets filter_policy (to NewBloomFilterPolicy(10)). The comment for the filter_policy says that "use the specified filter policy to reduce disk reads" and "Many applications will benefit ... from NewBloomFilterPolicy".

* 3 places set env.



Questions to cmumford@:

* What are benefits of setting paranoid_checks?

* Which use cases can push block_cache to its default limit (8 MiB)?

* What are negative effect of always setting filter_policy to NewBloomFilterPolicy?

* What are benefits of setting env?


I'm thinking of replacing all places that set performance-specific leveldb::Options with a call to leveldb_env::GetOptimalOptions(), which would return optimal per-platform options.

I.e.

  leveldb::Options options;
  options.max_open_files = 0;  // Use minimum.
  options.create_if_missing = true;
  options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;

becomes

  leveldb::Options options = leveldb_env::GetOptimalOptions();
  options.create_if_missing = true;



--------------------------------------------------------------------------------
Places where leveldb::DB::Open() is called:

src/components/leveldb/leveldb_service_impl.cc
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
    options.compression = leveldb::kSnappyCompression;
    options.env = MojoEnv (distinct from ChromiumEnv)

    options.create_if_missing = false (default)
    options.error_if_exists = false (default)
    options.paranoid_checks = false (default)
    options.write_buffer_size = 4194304 (default)
    options.max_open_files = 80 (default)
        (defaults are from see leveldb.mojom)


src/components/leveldb_proto/leveldb_database.cc
    leveldb_options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
    leveldb_options.max_open_files = 0

    leveldb_options.write_buffer_size = options.write_buffer_size;
    leveldb_options.block_cache = leveldb::NewLRUCache(options.read_cache_size))

    Used by:
    src/components/ntp_snippets/remote/remote_suggestions_database.cc
        options.write_buffer_size = 512 << 10
        options.read_cache_size = 512 << 10


src/content/browser/indexed_db/leveldb/leveldb_database.cc
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue
    options.write_buffer_size = leveldb_env::WriteBufferSize(...)
    options.paranoid_checks = true
    options.max_open_files = 80
    options.env = LevelDBEnv


src/extensions/browser/value_store/lazy_leveldb.cc
    open_options_.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue
    open_options_.paranoid_checks = true


src/google_apis/gcm/engine/gcm_store_impl.cc
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue
    options.paranoid_checks = true


src/chrome/browser/android/history_report/delta_file_backend_leveldb.cc
    options.max_open_files = 0


src/components/sync/model_impl/model_type_store_backend.cc
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue
    options.paranoid_checks = true
    options.env = nullptr or ModelTypeStoreBackend::CreateInMemoryEnv()


src/components/drive/resource_metadata_storage.cc
    options.max_open_files = 0
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue


src/storage/browser/fileapi/sandbox_origin_database.cc
    options.max_open_files = 0
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;


src/storage/browser/fileapi/sandbox_directory_database.cc
    options.max_open_files = 0
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue


src/components/sync/engine/attachments/on_disk_attachment_store.cc
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue


src/content/browser/notifications/notification_database.cc
    options.paranoid_checks = true
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue
    options.filter_policy = leveldb::NewBloomFilterPolicy(10)


src/content/browser/dom_storage/session_storage_database.cc
    options.max_open_files = 0
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
    options.write_buffer_size = 64 * 1024


src/content/browser/service_worker/service_worker_database.cc
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue
    options.env = leveldb::NewMemEnv or ServiceWorkerEnv


src/chrome/browser/sync_file_system/drive_backend/metadata_database.cc
    options.max_open_files = 0
    options.paranoid_checks = true
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue


src/chrome/browser/sync_file_system/local/local_file_change_tracker.cc
    options.max_open_files = 0
    options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue


src/components/data_reduction_proxy/core/browser/data_store_impl.cc
    options.paranoid_checks = true
    options.reuse_logs = false


src/chrome/browser/android/history_report/usage_reports_buffer_backend.cc
    options.max_open_files = 0

Apparently some places in Chrome potentially use leveldb::NewMemEnv(), which stores all DB files in memory:

LevelDBServiceImpl::OpenInMemory()

NotificationDatabase::Open()

LevelDBDatabase::OpenInMemory()

ServiceWorkerDatabase::LazyOpen()

The thing is that leveldb::Env doesn't have a concept of estimating memory usage, so memory usage of MemEnv is completely invisible to the memory reporting infrastructure.

We met with pwnall@, and it became clear that optimizing leveldb memory usage can come at a cost of increased IO (which is bad because it wears down flash).

So the plan now is to add memory and IO accounting for leveldb, see what real-world numbers are, and experiment from there.

Assigning to dskiba to drive this to completion.

In the general area of memory usage of leveldb, I wonder if it makes sense to always pass in a leveldb::NewLRUCache in the options, even if it would just be the default 8MB size, just to then be able to additionally listen for memory pressure events and call Prune() on the cache?

And of course passing in a Cache in the leveldb options (and thus having access to a pointer to the cache) will also make it possible to split up memory usage between the block cache and other sources of memory usage, to potentially get more detailed usage information in memory dumps.

mek@ so both #11 and #12 won't cause any change in IO? pwnall@ what do you think?

Just FYI: in one of our EM stories (visiting a news site) leveldatabase allocated 3.4MiB: go/mnnfk. That wasn't reported by leveldb MDP, but we'll soon have "leveldatabase" MDP (crrev.com/2855953002 and follow ups) that will report everything.

dskiba@: of course pruning the cache on memory pressure will cause more IO, but it might still make sense to do so anyway?

and splitting out the block cache memory from other memory used by leveldb would be purely a reporting difference, with no change in actual memory usage or access patterns.

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

commit 542a7c89b3e856ef642d3ff6352e661717172395
Author: dskiba <dskiba@chromium.org>
Date: Thu Jun 29 21:58:11 2017

leveldb: Add DBTracker for exposing databases to Chrome's memory-infra.

This CL adds DBTracker class that keeps track of all opened databases
and reports them to memory-infra (part of Chrome tracing). For database
to be tracked and reported it needs to be opened with leveldb_env::OpenDB()
function, which is a drop-in replacement for leveldb::DB::Open(). Next CL
will change all leveldb::DB::Open() invocations and add a presubmit check
to prevent adding new ones.

BUG= 711518 

Review-Url: https://codereview.chromium.org/2855953002
Cr-Commit-Position: refs/heads/master@{#483507}

[modify] https://crrev.com/542a7c89b3e856ef642d3ff6352e661717172395/third_party/leveldatabase/env_chromium.cc
[modify] https://crrev.com/542a7c89b3e856ef642d3ff6352e661717172395/third_party/leveldatabase/env_chromium.h
[modify] https://crrev.com/542a7c89b3e856ef642d3ff6352e661717172395/third_party/leveldatabase/env_chromium_unittest.cc

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

commit 947f697ef7cc43d8ae1d3ffea52a77b5771b8c22
Author: dskiba <dskiba@chromium.org>
Date: Thu Jul 06 03:35:56 2017

leveldb: Use base::RepeatingCallback in DBTracker.

This CL fixes accidental std::function usage in DBTracker.

BUG= 711518 

Review-Url: https://codereview.chromium.org/2964793002
Cr-Commit-Position: refs/heads/master@{#484456}

[modify] https://crrev.com/947f697ef7cc43d8ae1d3ffea52a77b5771b8c22/third_party/leveldatabase/env_chromium.cc
[modify] https://crrev.com/947f697ef7cc43d8ae1d3ffea52a77b5771b8c22/third_party/leveldatabase/env_chromium.h
[modify] https://crrev.com/947f697ef7cc43d8ae1d3ffea52a77b5771b8c22/third_party/leveldatabase/env_chromium_unittest.cc

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

commit 1c4e7620c24507de3bf3953af705a84c5f85e3c3
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Jul 14 23:25:35 2017

Use a shared LevelDB block cache for LevelDB instances.

There will be one shared block cache for all users of
LevelDB::Init, and a separate one for All IndexedDB
instances. The default is 8MB, but it's 512KB for
low-memory Android devices.

Previously, each instance would get its own block cache, and use
the default LevelDB settings for such a cache, which is 8MB (*)

(*) https://cs.chromium.org/chromium/src/third_party/leveldatabase/src/include/leveldb/options.h?gsn=Options&l=96

Bug:  711518 
Change-Id: I6a64467815489ae7695b8db9b9074170d9e56d94
Reviewed-on: https://chromium-review.googlesource.com/567583
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486934}
[modify] https://crrev.com/1c4e7620c24507de3bf3953af705a84c5f85e3c3/components/leveldb_proto/leveldb_database.cc
[modify] https://crrev.com/1c4e7620c24507de3bf3953af705a84c5f85e3c3/components/leveldb_proto/options.h
[modify] https://crrev.com/1c4e7620c24507de3bf3953af705a84c5f85e3c3/content/browser/indexed_db/leveldb/leveldb_database.cc

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

commit 1474c2bfd6194e04828472b5b0bda9b5a50912c8
Author: dskiba <dskiba@chromium.org>
Date: Thu Jul 20 02:19:24 2017

Use leveldb_env::OpenDB() to open leveldb databases.

This CL rewrites usages of leveldb::DB::Open() to leveldb_env::OpenDB(),
exposing memory usage of leveldb databases to Chrome's tracing.

The only API difference of leveldb_env::OpenDB() is related to how it returns
its result:

1. It returns (via output parameter) std::unique_ptr<leveldb::DB>, while
   leveldb::DB::Open() returns a raw leveldb::DB pointer.

2. It doesn't modify output parameter on failure, while leveldb::DB::Open()
   returns (writes) nullptr.

Both of these changes are consistent with Chrome's guidelines.

Under the hood leveldb_env::OpenDB() uses leveldb_env::DBTracker to open and
expose databases to Chrome's tracing. See crrev.com/2855953002 for details.

Finally, this CL does two more things:

1. Adds a presubmit check to enforce usage of leveldb_env::OpenDB().

2. Fixes existing memory dump providers that already dump some leveldb
   databases to properly attribute their memory usage. Further work on
   those MDPs is tracked by  crbug.com/735269 .

BUG= 711518 

Review-Url: https://codereview.chromium.org/2953473002
Cr-Commit-Position: refs/heads/master@{#488099}

[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/PRESUBMIT.py
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/android/history_report/delta_file_backend_leveldb.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/android/history_report/usage_reports_buffer_backend.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/leveldb_wrapper_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/metadata_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/metadata_database_index_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/metadata_db_migration_util_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/drive_backend/register_app_task_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/chrome/browser/sync_file_system/local/local_file_change_tracker.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/data_reduction_proxy/core/browser/data_store_impl.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/drive/resource_metadata_storage.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/leveldb/leveldb_database_impl.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/leveldb/leveldb_service_impl.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/leveldb_proto/leveldb_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/leveldb_proto/proto_database_impl_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/sync/engine/attachments/on_disk_attachment_store.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/sync/engine/attachments/on_disk_attachment_store_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/components/sync/model_impl/model_type_store_backend.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/content/browser/dom_storage/local_storage_context_mojo_unittest.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/content/browser/dom_storage/session_storage_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/content/browser/dom_storage/session_storage_database.h
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/content/browser/indexed_db/leveldb/leveldb_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/content/browser/notifications/notification_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/content/browser/service_worker/service_worker_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/extensions/browser/value_store/lazy_leveldb.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/extensions/browser/value_store/leveldb_value_store.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/google_apis/gcm/engine/gcm_store_impl.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/storage/browser/fileapi/sandbox_directory_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/storage/browser/fileapi/sandbox_origin_database.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/third_party/leveldatabase/env_chromium.cc
[modify] https://crrev.com/1474c2bfd6194e04828472b5b0bda9b5a50912c8/third_party/leveldatabase/env_chromium.h

Sorry, I missed the questions in #c6 above. My answers:

* What are benefits of setting paranoid_checks?

  This is to detect corruption via verifying checksums when reading data from tables. It doesn't cause more data to be read (thus filling caches).

* Which use cases can push block_cache to its default limit (8 MiB)?

  8MB of table block reads, all of which miss the cache.

* What are negative effect of always setting filter_policy to NewBloomFilterPolicy?
  
  Approximately 10 bits of RAM used per key, and also time to read (and verify) the filter block during open.

* What are benefits of setting env?

  Merely that the caller gets to have "their" Env used by that database. If it is left to the default (null) value then leveldb just calls leveldb::Env::Default() to get the default env - which we implement in env_chromium.cc
  

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

commit 33b762b8f0cebfaa27fe6b07d24b97c437b26f7d
Author: Chris Mumford <cmumford@chromium.org>
Date: Thu Sep 14 19:45:46 2017

leveldb: Have GetOrCreateAllocatorDump fail on untracked db.

The callers of DBTracker::GetOrCreateAllocatorDump assume that the
DBTracker is dumping the passed |db|. Adding a DCHECK to detect
adding an edge to a nonexistent dump.

BUG= 711518 

Change-Id: I5b6fc966eb56d97436add9b24117f6b4539c2690
Reviewed-on: https://chromium-review.googlesource.com/650838
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502019}
[modify] https://crrev.com/33b762b8f0cebfaa27fe6b07d24b97c437b26f7d/third_party/leveldatabase/env_chromium.cc
[modify] https://crrev.com/33b762b8f0cebfaa27fe6b07d24b97c437b26f7d/third_party/leveldatabase/env_chromium.h
[modify] https://crrev.com/33b762b8f0cebfaa27fe6b07d24b97c437b26f7d/third_party/leveldatabase/env_chromium_unittest.cc

cmumford@ is working on this.

As far as I know, we've wrapped up all the work we were planning to do here.

cmumford@: Feel free to unassign yourself and make the bug available if I'm right.

Lots of work was done to minimize (i.e. tune) leveldb's memory use on Android, and the original "ask" of this bug was a little vague. I think that all (or at least most) of what was needed has been done so I'm going to mark this as fixed.

If I've mistakenly closed this, and in fact there is still more work needed that I've overlooked in the discussion above please feel free to reopen. However, a better idea might be to create a new issue with a specific request.