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

Issue 697612 link

Starred by 2 users
Status: Fixed
Owner:
mek@chromium.org
Closed: Apr 2017
Cc:
cmumford@chromium.org
█ e...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 586194



Sign in to add a comment

mojo LevelDB service should not create a separate leveldb environment per db connection

Project Member Reported by mek@chromium.org, Mar 1 2017 
Currently the mojo LevelDB service creates a new leveldb environment for each connection made to a database. This has several problems:

- each env will have its own background thread, causing potentially many background threads all trying to run compaction at the same time etc

- MojoEnv derives from ChromiumEnv, yet ChromiumEnv doesn't support being destructed. This means that (if we fix the mojo leveldb service to not destroy it's envs) we'd leak an env instance for every database connection

- something about file locks not working properly, but probably that should (or maybe is already) solved at the level of the mojo file service?


Not sure how to best solve this though. Currently when opening a connection to a database you specify the filesystem.mojom.Directory you want the files to be created in, and the various env methods need to know what this directory is to be able to interpret the paths it is given.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 13 2017 

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

commit 6bfb96f0eb4350010c8d9c29fb40408212536bcc
Author: mek <mek@chromium.org>
Date: Thu Apr 13 21:36:13 2017

leveldb::MojoEnv should not inherit from ChromiumEnv.

This changes leveldb::MojoEnv to no longer inherit from ChromiumEnv
since it overrides nearly every method of that class anyway. Also
rather than having the one-background-thread-per-env that ChromiumEnv
has this uses base::PostTask to post background tasks to the global task
scheduler.

Also got rid of being able to specify a custom environment name
for MojoEnv. MojoEnv doesn't (and hasn't) actually emit any of
the ChromiumEnv histograms. In a follow-up CL I'll add histograms
to MojoEnv similar to the ones that ChromiumEnv emits, but for now
it doesn't make sense to have these histograms defined.

BUG= 697612 

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

[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/components/leveldb/env_mojo.cc
[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/components/leveldb/env_mojo.h
[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/components/leveldb/leveldb_service_impl.cc
[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/components/leveldb/leveldb_service_impl.h
[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/components/leveldb/public/interfaces/leveldb.mojom
[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/content/browser/dom_storage/local_storage_context_mojo.cc
[modify] https://crrev.com/6bfb96f0eb4350010c8d9c29fb40408212536bcc/tools/metrics/histograms/histograms.xml

Comment 2 by mek@chromium.org, Apr 13 2017

Status: Fixed (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 24 2017 

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

commit 9a16c13f62099154b115bf2e3464efb075b9d3f9
Author: mek <mek@chromium.org>
Date: Mon Apr 24 22:04:04 2017

Use correct task traits for mojo leveldb env background tasks.

These background tasks can do file operations, which might involve
blocking using base::WaitableEvents as well as other blocking in
leveldb code itself.

BUG= 697612 

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

[modify] https://crrev.com/9a16c13f62099154b115bf2e3464efb075b9d3f9/components/leveldb/env_mojo.cc
►

Sign in to add a comment