Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(16)

Issue 2722293002: Fix lifetime of leveldb::MojoEnv instances. (Closed)

Created:
3 years, 9 months ago by Marijn Kruisselbrink
Modified:
3 years, 8 months ago
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix lifetime of leveldb::MojoEnv instances. ChromiumEnv does not support being destructed, so since MojoEnv derives from ChromiumEnv it can't be destroyed either. This changes the lifetime of MojoEnv instances to no longer have one instance for each database connection, but instead have a global instance (which leaks). To make this possibly, MojoEnv uses "virtual" paths to be able to look up the correct mojo Directory instance to use for a particular database connection. Also added a NOTREACHED to ~ChromiumEnv, as the comment there suggested, since nothing seem to be broken by this change. Finally also got rid of being able to specify a custom environment name for MojoEnv. If eventually we do want different histograms for different leveldb connections, the same virtual path trick could be used to look up the histogram name to use for particular connections. But that would be a fairly invasive change for now. Also MojoEnv doesn't actually emit any of the ChromiumEnv histograms yet, since it overrides (nearly) every method that would emit histograms. In a follow-up CL I'll add the histogram logging to MojoEnv. BUG=697612

Patch Set 1 #

Patch Set 2 : hack to see if anything other than mojo stuff fails #

Patch Set 3 : refactor mojoenv #

Patch Set 4 : undo incorrect change #

Patch Set 5 : fix directory lifetime #

Patch Set 6 : fix memory backed leveldb lifetime #

Patch Set 7 : minor cleanups #

Total comments: 9

Patch Set 8 : make registering directories nicer #

Patch Set 9 : pass in leveldb environment to leveldb service, rather than creating it internally #

Patch Set 10 : annotate leaks #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -127 lines) Patch
M components/leveldb/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M components/leveldb/env_mojo.h View 1 2 3 4 5 6 7 8 3 chunks +32 lines, -5 lines 1 comment Download
M components/leveldb/env_mojo.cc View 1 2 3 4 5 6 7 8 5 chunks +137 lines, -37 lines 0 comments Download
M components/leveldb/leveldb_app.h View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -0 lines 4 comments Download
M components/leveldb/leveldb_app.cc View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -3 lines 0 comments Download
M components/leveldb/leveldb_database_impl.h View 1 2 3 4 5 6 7 3 chunks +8 lines, -4 lines 0 comments Download
M components/leveldb/leveldb_database_impl.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -2 lines 0 comments Download
M components/leveldb/leveldb_mojo_proxy.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M components/leveldb/leveldb_service_impl.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -9 lines 0 comments Download
M components/leveldb/leveldb_service_impl.cc View 1 2 3 4 5 6 7 8 3 chunks +13 lines, -27 lines 0 comments Download
M components/leveldb/public/interfaces/leveldb.mojom View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M content/browser/browser_context.cc View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -4 lines 0 comments Download
M content/browser/dom_storage/local_storage_context_mojo.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/dom_storage/local_storage_context_mojo_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +8 lines, -4 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M services/file/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M services/file/file_service.h View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -2 lines 0 comments Download
M services/file/file_service.cc View 1 2 3 4 5 6 7 8 4 chunks +13 lines, -15 lines 0 comments Download
M third_party/leveldatabase/env_chromium.cc View 1 2 3 4 1 chunk +3 lines, -3 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 4 chunks +5 lines, -6 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 63 (51 generated)
Marijn Kruisselbrink
3 years, 9 months ago (2017-03-15 20:40:46 UTC) #26
Elliot Glaysher
lgtm
3 years, 9 months ago (2017-03-15 22:04:34 UTC) #29
Marijn Kruisselbrink
michaeln: since cmumford is OOO, would you mind taking a look (as third_party/leveldatabase OWNER, and ...
3 years, 9 months ago (2017-03-16 18:08:58 UTC) #31
michaeln
https://codereview.chromium.org/2722293002/diff/120001/components/leveldb/env_mojo.cc File components/leveldb/env_mojo.cc (right): https://codereview.chromium.org/2722293002/diff/120001/components/leveldb/env_mojo.cc#newcode231 components/leveldb/env_mojo.cc:231: if (env->thread_->task_runner() == file_task_runner) I think this does ptr ...
3 years, 9 months ago (2017-03-27 20:24:11 UTC) #32
Marijn Kruisselbrink
https://codereview.chromium.org/2722293002/diff/120001/components/leveldb/env_mojo.cc File components/leveldb/env_mojo.cc (right): https://codereview.chromium.org/2722293002/diff/120001/components/leveldb/env_mojo.cc#newcode231 components/leveldb/env_mojo.cc:231: if (env->thread_->task_runner() == file_task_runner) On 2017/03/27 at 20:24:10, michaeln ...
3 years, 8 months ago (2017-03-31 23:26:24 UTC) #33
michaeln
lmk if you want me to look at the new snapshot, i'm not sure if ...
3 years, 8 months ago (2017-04-10 23:02:09 UTC) #38
Marijn Kruisselbrink
On 2017/04/10 at 23:02:09, michaeln wrote: > lmk if you want me to look at ...
3 years, 8 months ago (2017-04-12 21:16:04 UTC) #56
cmumford
https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/leveldb_app.h File components/leveldb/leveldb_app.h (right): https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/leveldb_app.h#newcode46 components/leveldb/leveldb_app.h:46: // env_ is owned by LevelDBApp (since it depends ...
3 years, 8 months ago (2017-04-12 21:51:34 UTC) #57
Marijn Kruisselbrink
https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/leveldb_app.h File components/leveldb/leveldb_app.h (right): https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/leveldb_app.h#newcode46 components/leveldb/leveldb_app.h:46: // env_ is owned by LevelDBApp (since it depends ...
3 years, 8 months ago (2017-04-12 22:00:34 UTC) #60
cmumford
https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/env_mojo.h File components/leveldb/env_mojo.h (right): https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/env_mojo.h#newcode47 components/leveldb/env_mojo.h:47: // Overridden from leveldb_env::EnvChromium: I know it was there ...
3 years, 8 months ago (2017-04-12 23:06:39 UTC) #61
Marijn Kruisselbrink
https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/leveldb_app.h File components/leveldb/leveldb_app.h (right): https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/leveldb_app.h#newcode46 components/leveldb/leveldb_app.h:46: // env_ is owned by LevelDBApp (since it depends ...
3 years, 8 months ago (2017-04-12 23:46:22 UTC) #62
Marijn Kruisselbrink
3 years, 8 months ago (2017-04-13 18:04:08 UTC) #63
On 2017/04/12 at 23:46:22, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/lev...
> File components/leveldb/leveldb_app.h (right):
> 
>
https://codereview.chromium.org/2722293002/diff/240001/components/leveldb/lev...
> components/leveldb/leveldb_app.h:46: // env_ is owned by LevelDBApp (since it
depends on the file_thread_ above),
> On 2017/04/12 at 23:06:39, cmumford wrote:
> > That's right.
> > 
> > Also, this was prior to your change, but I don't see any reason for MojoEnv
to derive from ChromiumEnv. It overrides all but four functions, and doesn't get
any of it's UMA logging functionality in ChromiumEnv. I think that using
leveldb::Env would be simpler. You would need to implement StartThread, etc.,
and as you mention join the background thread before destruction.
> 
> Yeah, maybe that's the way to go... If we do that, would there still be any
reason for this CL? Or would having a separate MojoEnv for each connection be
just fine (other than the slight inefficiency of having a different background
thread for each connection)?

Okay, new CL in https://codereview.chromium.org/2822503002 that changes MojoEnv
to no longer derive from ChromiumEnv, which I think eliminates the need for this
CL, as at that point I don't think there are any issues with having a separate
MojoEnv instance for each leveldb connection.

Powered by Google App Engine
This is Rietveld 408576698