Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2855953002:
leveldb: Add DBTracker for exposing databases to Chrome's memory-infra. (Closed)
|
Created:
3 years, 7 months ago by DmitrySkiba Modified:
3 years, 4 months ago CC:
awdf+watch_chromium.org, blink-worker-reviews_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, cmumford, darin-cc_chromium.org, extensions-reviews_chromium.org, fukino+watch_chromium.org, horo+watch_chromium.org, jam, johnme+watch_chromium.org, jsbell+idb_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, kinuko+fileapi, michaeln, mlamouri+watch-notifications_chromium.org, nhiroki, oka+watch_chromium.org, Peter Beverloo, serviceworker-reviews, shimazu+serviceworker_chromium.org, sync-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, tzik, yamaguchi+watch_chromium.org, zea+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionleveldb: 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}
Committed: https://chromium.googlesource.com/chromium/src/+/542a7c89b3e856ef642d3ff6352e661717172395
Patch Set 1 #
Total comments: 1
Patch Set 2 : Finalize #Patch Set 3 : Remove iterator wrapper #Patch Set 4 : Add unittests #
Total comments: 9
Patch Set 5 : Address comments #
Total comments: 38
Patch Set 6 : Address comments #
Total comments: 9
Patch Set 7 : Don't include inttypes directly #Patch Set 8 : OpenDB and friends #
Total comments: 2
Patch Set 9 : Fix Windows #Patch Set 10 : Address comments #
Total comments: 1
Messages
Total messages: 35 (18 generated)
ssid@chromium.org changed reviewers: + ssid@chromium.org
https://codereview.chromium.org/2855953002/diff/1/third_party/leveldatabase/e... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/1/third_party/leveldatabase/e... third_party/leveldatabase/env_chromium.cc:1147: bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args, If we are adding all the databases here then we should remove the dump providers that already exist: IndexedDB, leveldb_proto and LevelDBValueStore. Else this will double count the same memory. Malloc total could become negative because we subtract this memory twice, if leveldb was the cause of half the malloc usage.
Description was changed from ========== [EXPERIMENTAL] Add and use leveldb_env::DBRegistry. BUG=711518 ========== to ========== leveldb: Add DBRegistry. This CL adds DBRegistry 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 DBRegistry::Open method, which is a drop-in replacement for DB::Open. Next CL will change all DB::Open invocations and add a presubmit check to prevent adding new ones. BUG=711518 ==========
dskiba@chromium.org changed reviewers: + pwnall@chromium.org
PTAL. The follow-up CL that changes DB::Open calls in Chrome is here: https://codereview.chromium.org/2953473002/
mek@chromium.org changed reviewers: + mek@chromium.org
some drive-by comments https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1181: "leveldatabase/0x%" PRIXPTR, reinterpret_cast<uintptr_t>(db)); nit: should this be leveldb/ something to be consistent with existing leveldb memory dump providers? Or is this intentionally different? https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:244: static leveldb::Status Open(const leveldb::Options& options, drive-by comment: presumably this is supposed to also replace things like the MemoryDumpProvider in https://cs.chromium.org/chromium/src/components/leveldb/leveldb_database_impl..., in which case you'll need some way of optionally passing in a shared global memory dump ID associated with the DB, to then allow linking leveldb memory usage with whatever part of the code is using leveldb
memory-infra bits looks good. https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1181: "leveldatabase/0x%" PRIXPTR, reinterpret_cast<uintptr_t>(db)); On 2017/06/20 23:38:52, Marijn Kruisselbrink wrote: > nit: should this be leveldb/ something to be consistent with existing leveldb > memory dump providers? Or is this intentionally different? I think it is intentional. How about leveldb_registry? It might be confuding to explain to users why we have "leveldb" and "leveldatabase" in the dump provider column. Or if you are planning to fix all providers with different names, then it's good. https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1220: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( You cannot unregister dump providers without task runner. you need to use UnregisterDumpProviderAndDeleteSoon. This is because MemoryDumpManager does not use locks and another thread might be calling OnMemoryDump while this is deleted. Anyways this is singleton and this problem does not exist. So, let's remove this call instead of having a misleading call? https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:244: static leveldb::Status Open(const leveldb::Options& options, On 2017/06/20 23:38:52, Marijn Kruisselbrink wrote: > drive-by comment: presumably this is supposed to also replace things like the > MemoryDumpProvider in > https://cs.chromium.org/chromium/src/components/leveldb/leveldb_database_impl..., > in which case you'll need some way of optionally passing in a shared global > memory dump ID associated with the DB, to then allow linking leveldb memory > usage with whatever part of the code is using leveldb I don't think we want to replace the existing providers. This is because we will lose contextual information about the database itself. Like the ValueStore provider might add extra details like number of values stored or local storage might add information like URL name. I think the plan is to just use AddOwnershipEdge between this dump and the dump created by leveldb_database_impl and remove the AddSuballocation("malloc") call from the leveldb_database_impl. This would account for double counting.
https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:244: static leveldb::Status Open(const leveldb::Options& options, On 2017/06/20 at 23:52:13, ssid wrote: > On 2017/06/20 23:38:52, Marijn Kruisselbrink wrote: > > drive-by comment: presumably this is supposed to also replace things like the > > MemoryDumpProvider in > > https://cs.chromium.org/chromium/src/components/leveldb/leveldb_database_impl..., > > in which case you'll need some way of optionally passing in a shared global > > memory dump ID associated with the DB, to then allow linking leveldb memory > > usage with whatever part of the code is using leveldb > > I don't think we want to replace the existing providers. This is because we will lose contextual information about the database itself. Like the ValueStore provider might add extra details like number of values stored or local storage might add information like URL name. > > I think the plan is to just use AddOwnershipEdge between this dump and the dump created by leveldb_database_impl and remove the AddSuballocation("malloc") call from the leveldb_database_impl. This would account for double counting. Ah, okay, makes sense. So there will be some magic way for the other database dumps to create the ownership edge between it and what is created by DBRegistry?
https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1181: "leveldatabase/0x%" PRIXPTR, reinterpret_cast<uintptr_t>(db)); On 2017/06/20 23:38:52, Marijn Kruisselbrink wrote: > nit: should this be leveldb/ something to be consistent with existing leveldb > memory dump providers? Or is this intentionally different? Yes, the name is intentionally different. Some of the providers that we have now (e.g. leveldb_proto) will be completely superseded by this one. The idea is to make this provider the root one, and use database pointer as an ID. We'll need to expose MDP name out of DBRegistry at some point (maybe even in this CL). https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:244: static leveldb::Status Open(const leveldb::Options& options, On 2017/06/20 23:38:52, Marijn Kruisselbrink wrote: > drive-by comment: presumably this is supposed to also replace things like the > MemoryDumpProvider in > https://cs.chromium.org/chromium/src/components/leveldb/leveldb_database_impl..., > in which case you'll need some way of optionally passing in a shared global > memory dump ID associated with the DB, to then allow linking leveldb memory > usage with whatever part of the code is using leveldb Yup, that's pretty much what we discussed with ssid@ at some point. It seems that if we make ID to be DB pointer everything is going to work - leveldatabase will use DB pointer, and mojo will transfer it as an ID to clients, which will use it to add an allocation edge. What do you think? Let's continue the discussion in crbug.com/735269.
https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/60001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1220: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( On 2017/06/20 23:52:13, ssid wrote: > You cannot unregister dump providers without task runner. > you need to use UnregisterDumpProviderAndDeleteSoon. This is because > MemoryDumpManager does not use locks and another thread might be calling > OnMemoryDump while this is deleted. > > Anyways this is singleton and this problem does not exist. So, let's remove this > call instead of having a misleading call? Ouch, I didn't know about that. I wonder if UnregisterDumpProvider() can be renamed to something to indicate it works only with MDPs that have task_runners? In this case I'll just remove the call.
Thank you for working on this! https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1110: DBWrapperImpl(const std::string name, leveldb::DB* db) I think it's cleaner to pass a DBRegistry pointer to the constructor, store it in the instance, and call UnregisterDatabase on it. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1174: // DBRegistry MemoryDumpProvider implementation. Declared here to minimize Both sentences in the comment seem redundant with the code. Can you please remove, and possibly replace with an explanation of what this tracks? Example output might be sufficient. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1225: static DBRegistry* instance = new DBRegistry(); Is this threadsafe? (Asking because DBRegistry's comment says the class is threadsafe.) I haven't kept up with how Chromium deals with thread safety, but I vaguely remember that more ceremony was involved. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:240: class DBRegistry { I'd prefer that Open() and VisitOpenDatabases() become functions in the leveldb_env namespace, so calls look like leveldb_env::Open(). leveldb_env::VisitOpenDatabases can proxy to the DBRegistry singleton instance, which becomes an implementation detail. I prefer the name DBTracker to DBRegistry, because I think it conveys the purpose better. Also, I'd prefer that Register / Unregister become DatabaseOpened() / DatabaseClosed(), as this clarifies when they're supposed to be called. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:242: // Opens a database. Internally calls leveldb::DB::Open(options, name) Can you please write the docs from the perspective of a class user? In this case, I think it makes more sense to state that the returned instances are tracked by the registry so they're available in VisitDatabases. I don't think the class user cares how the wrapping works, so there's no reason to commit to specifics in the docs. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:254: virtual const std::string& name() const = 0; Do you envision adding more functionality here, such as iterating over a database's open iterators? If not, it might be simpler to add the name parameter to the DatabaseVisitor function and remove this interface altogether. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:257: using DatabaseVisitor = std::function<void(DBWrapper*)>; Visitor? https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:262: void VisitDatabases(const DatabaseVisitor& visitor); VisitOpenDatabases? https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:266: class MDP; Can you please use the full name MemoryDumpProvider? I don't think this is an obvious abbreviation for people outside memory infra. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium_unittest.cc (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:194: TEST(ChromiumEnv, DBRegistryOpen) { Thank you for tests! FYI, leveldb tests don't run on the CQ yet. Please make sure they pass on your machine. ninja -C out/Default/ leveldb_env_test out/Default/leveldb_env_test https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:207: // Open a new database using DBRegistry::Open, write some data Periods at the end of sentences? https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:261: size_t visited_db_count; Can you initialize this to 0, so a reader doesn't have to wonder why it's not initialized? https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:266: visited_db_count += 1; This algorithm doesn't catch the case where the visitor yields the same db multiple times. Here are two solutions that should not add much complexity. 1) Hav an array of flags that you memset to zero before each iteration. 2) Instead of the NamedDB struct, you start with an of C strings that have names. You create a map<string, DB*> out of that. Each visit test creates a copy of the map. When a DB is visited, you check that the name exists in the map and its value matches the DB* vended by the visitor, and then you remove the name from the map. At the end, you check that the map is empty. How about the following alternative? https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:278: // Close couple databases a couple of?
Also: this CL's purpose appears to be exposing LevelDB memory usage to the memory infrastructure. "Add DBRegistry" doesn't reflect this. Can you please use something like "Expose LevelDB memory usage to the Chrome's infrastructure."
https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1110: DBWrapperImpl(const std::string name, leveldb::DB* db) On 2017/06/22 02:50:22, pwnall wrote: > I think it's cleaner to pass a DBRegistry pointer to the constructor, store it > in the instance, and call UnregisterDatabase on it. Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1174: // DBRegistry MemoryDumpProvider implementation. Declared here to minimize On 2017/06/22 02:50:22, pwnall wrote: > Both sentences in the comment seem redundant with the code. Can you please > remove, and possibly replace with an explanation of what this tracks? Example > output might be sufficient. Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.cc:1225: static DBRegistry* instance = new DBRegistry(); On 2017/06/22 02:50:22, pwnall wrote: > Is this threadsafe? (Asking because DBRegistry's comment says the class is > threadsafe.) > > I haven't kept up with how Chromium deals with thread safety, but I vaguely > remember that more ceremony was involved. Yes, Chrome switched back to using treadsafe static locals (since MSVC 2015 started supporting them). This is now the pattern. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:240: class DBRegistry { On 2017/06/22 02:50:22, pwnall wrote: > I'd prefer that Open() and VisitOpenDatabases() become functions in the > leveldb_env namespace, so calls look like leveldb_env::Open(). > leveldb_env::VisitOpenDatabases can proxy to the DBRegistry singleton instance, > which becomes an implementation detail. > > I prefer the name DBTracker to DBRegistry, because I think it conveys the > purpose better. Also, I'd prefer that Register / Unregister become > DatabaseOpened() / DatabaseClosed(), as this clarifies when they're supposed to > be called. Renaming: done. I'm not sure about free-standing functions. In my view the class serves as a clustering point for all related functionality. I.e. if you see that DBTracker::Open() is called, you know that there are probably more stuff in DBTracker. leveldb_env::Open() doesn't tell much - what is opened, and why not DB::Open? DBTracker::Open() provides more context. Besides, the class provides convenient and unambiguous place to add new functionality. For example along with your comments I've added GetMemoryDumpName() API so that other MDPs could either provide additional per-database info, or reference database dumps. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:242: // Opens a database. Internally calls leveldb::DB::Open(options, name) On 2017/06/22 02:50:22, pwnall wrote: > Can you please write the docs from the perspective of a class user? > > In this case, I think it makes more sense to state that the returned instances > are tracked by the registry so they're available in VisitDatabases. I don't > think the class user cares how the wrapping works, so there's no reason to > commit to specifics in the docs. Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:254: virtual const std::string& name() const = 0; On 2017/06/22 02:50:22, pwnall wrote: > Do you envision adding more functionality here, such as iterating over a > database's open iterators? > > If not, it might be simpler to add the name parameter to the DatabaseVisitor > function and remove this interface altogether. Yes, more stuff is planned: visiting iterators, exposing caches (similarly to https://chromium-review.googlesource.com/543439). https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:257: using DatabaseVisitor = std::function<void(DBWrapper*)>; On 2017/06/22 02:50:22, pwnall wrote: > Visitor? I think that if method is called VisitDatabases(), then 'DatabaseVisitor' makes more sense. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:262: void VisitDatabases(const DatabaseVisitor& visitor); On 2017/06/22 02:50:22, pwnall wrote: > VisitOpenDatabases? I feel that 'Open' is redundant, since it's the only option (i.e. we're not supporting visiting 'Closed' databases). https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:266: class MDP; On 2017/06/22 02:50:22, pwnall wrote: > Can you please use the full name MemoryDumpProvider? I don't think this is an > obvious abbreviation for people outside memory infra. Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium_unittest.cc (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:194: TEST(ChromiumEnv, DBRegistryOpen) { On 2017/06/22 02:50:22, pwnall wrote: > Thank you for tests! > > FYI, leveldb tests don't run on the CQ yet. Please make sure they pass on your > machine. > > ninja -C out/Default/ leveldb_env_test > out/Default/leveldb_env_test Acknowledged. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:207: // Open a new database using DBRegistry::Open, write some data On 2017/06/22 02:50:22, pwnall wrote: > Periods at the end of sentences? Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:261: size_t visited_db_count; On 2017/06/22 02:50:22, pwnall wrote: > Can you initialize this to 0, so a reader doesn't have to wonder why it's not > initialized? Acknowledged. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:266: visited_db_count += 1; On 2017/06/22 02:50:22, pwnall wrote: > This algorithm doesn't catch the case where the visitor yields the same db > multiple times. > > Here are two solutions that should not add much complexity. > > 1) Hav an array of flags that you memset to zero before each iteration. > 2) Instead of the NamedDB struct, you start with an of C strings that have > names. You create a map<string, DB*> out of that. Each visit test creates a copy > of the map. When a DB is visited, you check that the name exists in the map and > its value matches the DB* vended by the visitor, and then you remove the name > from the map. At the end, you check that the map is empty. > > How about the following alternative? Good catch. Redid the test. The motivation behind DBInfo is to be able to add (and check) more info as it's added to DBWrapper. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:278: // Close couple databases On 2017/06/22 02:50:22, pwnall wrote: > a couple of? Done.
On 2017/06/22 02:54:41, pwnall wrote: > Also: this CL's purpose appears to be exposing LevelDB memory usage to the > memory infrastructure. "Add DBRegistry" doesn't reflect this. Can you please use > something like "Expose LevelDB memory usage to the Chrome's infrastructure." This CL by itself doesn't expose anything, it just adds a facility to do so. Next CL (https://codereview.chromium.org/2953473002) will replace DB::Open with DBTracker::Open, and that will expose DBs to memory-infra. I'll reword CL title to be something in between.
Description was changed from ========== leveldb: Add DBRegistry. This CL adds DBRegistry 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 DBRegistry::Open method, which is a drop-in replacement for DB::Open. Next CL will change all DB::Open invocations and add a presubmit check to prevent adding new ones. BUG=711518 ========== to ========== 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 DBTracker::Open method, which is a drop-in replacement for DB::Open. Next CL will change all DB::Open invocations and add a presubmit check to prevent adding new ones. BUG=711518 ==========
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Thank you for the revisions! I think this CL is getting quite close. In the interest of minimizing round-trips, can you please check that the next version builds on CQ? https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:240: class DBRegistry { On 2017/06/26 20:45:18, DmitrySkiba wrote: > On 2017/06/22 02:50:22, pwnall wrote: > > I'd prefer that Open() and VisitOpenDatabases() become functions in the > > leveldb_env namespace, so calls look like leveldb_env::Open(). > > leveldb_env::VisitOpenDatabases can proxy to the DBRegistry singleton > instance, > > which becomes an implementation detail. > > > > I prefer the name DBTracker to DBRegistry, because I think it conveys the > > purpose better. Also, I'd prefer that Register / Unregister become > > DatabaseOpened() / DatabaseClosed(), as this clarifies when they're supposed > to > > be called. > > Renaming: done. > > I'm not sure about free-standing functions. In my view the class serves as a > clustering point for all related functionality. I.e. if you see that > DBTracker::Open() is called, you know that there are probably more stuff in > DBTracker. > > leveldb_env::Open() doesn't tell much - what is opened, and why not DB::Open? > DBTracker::Open() provides more context. This comment (that I agree with) suggests that leveldb_env::OpenDatabase() / OpenDB() would be better names than a naked Open(). I think that the context given by DBTracker is unnecessary mental overhead for most readers, who just want to get a leveldb::DB instance and get on with their lives. > Besides, the class provides convenient and unambiguous place to add new > functionality. For example along with your comments I've added > GetMemoryDumpName() API so that other MDPs could either provide additional > per-database info, or reference database dumps. I'm not opposed to having the class for tracking-related functionality. Open / OpenDatabase / OpenDB should definitely be exposed in the leveldb namespace, as the tracking should be an implementation detail. VisitOpenDatabases can remain in the tracker class, if that makes more sense. Also, in the interest of keeping CLs manageable in terms of size and review time, please avoid adding more stuff after the review has started. Dependent CLs FTW :) https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:254: virtual const std::string& name() const = 0; On 2017/06/26 20:45:19, DmitrySkiba wrote: > On 2017/06/22 02:50:22, pwnall wrote: > > Do you envision adding more functionality here, such as iterating over a > > database's open iterators? > > > > If not, it might be simpler to add the name parameter to the DatabaseVisitor > > function and remove this interface altogether. > > Yes, more stuff is planned: visiting iterators, exposing caches (similarly to > https://chromium-review.googlesource.com/543439). How about TrackedDB instead of DB for the name, then? I'm not a big fan of empty words like wrapper / proxy / decorator in APIs. The class comment can then state that the class exposes the information tracked by DBTracker. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:257: using DatabaseVisitor = std::function<void(DBWrapper*)>; On 2017/06/26 20:45:18, DmitrySkiba wrote: > On 2017/06/22 02:50:22, pwnall wrote: > > Visitor? > > I think that if method is called VisitDatabases(), then 'DatabaseVisitor' makes > more sense. I thought Visitor is acceptable if this is the only visitor, but I suppose we'll have an iterator visitor as well at some point in the near future. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:262: void VisitDatabases(const DatabaseVisitor& visitor); On 2017/06/26 20:45:19, DmitrySkiba wrote: > On 2017/06/22 02:50:22, pwnall wrote: > > VisitOpenDatabases? > > I feel that 'Open' is redundant, since it's the only option (i.e. we're not > supporting visiting 'Closed' databases). It is, once you've thought things through. However, having it in is useful for a reader that isn't fully familiar with the leveldb::DB lifecycle. For example, if I forget how leveldb works, I'll wonder if it's possible to have DB instances that have been created but not "opened" yet, and instances that have been closed but not released, and I'll wonder what gets visited and what doesn't. I don't want to force readers to remember how LevelDB works to be able to reason through this code. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium_unittest.cc (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:266: visited_db_count += 1; On 2017/06/26 20:45:19, DmitrySkiba wrote: > On 2017/06/22 02:50:22, pwnall wrote: > > This algorithm doesn't catch the case where the visitor yields the same db > > multiple times. > > > > Here are two solutions that should not add much complexity. > > > > 1) Hav an array of flags that you memset to zero before each iteration. > > 2) Instead of the NamedDB struct, you start with an of C strings that have > > names. You create a map<string, DB*> out of that. Each visit test creates a > copy > > of the map. When a DB is visited, you check that the name exists in the map > and > > its value matches the DB* vended by the visitor, and then you remove the name > > from the map. At the end, you check that the map is empty. > > > > How about the following alternative? > > Good catch. Redid the test. The motivation behind DBInfo is to be able to add > (and check) more info as it's added to DBWrapper. Thanks for explaining! It makes sense to keep it, then. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium_unittest.cc:278: // Close couple databases On 2017/06/26 20:45:19, DmitrySkiba wrote: > On 2017/06/22 02:50:22, pwnall wrote: > > a couple of? > > Done. Almost? You still need the "a". https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:238: // Keeps track of live databases (opened via Open() method) and "Tracks database open via .... " seems shorter. IIUC, while this class exists to expose the information to memory-infra, it doens't do the exposing directly, so I'd remove that sentence. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:252: static std::string GetMemoryDumpName(leveldb::DB* db); FWIW, this method makes DBTracker depend on memory-infra, conceptually. Without it, DBTracker is an abstract layer that helps inspect LevelDB. I don't know how much you value this layering, but I figured it's worth noting. Separately, it might be worth noting if this only works correctly when used with leveldb::DB instances produced by Open(). IIUC, that seems to be the case. Assuming I'm correct, I think it'd also be worth adding a DCHECK verifying the pre-condition. (the best solution I can think of is checking that the DB instance is in the linked list) Given the extra complexity around this method, I think it'd make more sense to introduce it in a separate CL that adds at least one MDP user, so it's easier to see what the best design would be. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:268: // databases themselves). Note that visiting order is not specified. "The databases may be visited in an arbitrary order." to also waive the guarantee for a consistent order? https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... File third_party/leveldatabase/env_chromium_unittest.cc (right): https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium_unittest.cc:271: auto visit_databases = []() -> VisitedDBs { I'd prefer that you use a ChromiumEnvDBTrackerTest test fixture class and move the types and VisitDatabases / AssertDatabasesLive there.
The CQ bit was checked by dskiba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The latest patchset introduces leveldb_env::OpenDB() (as per comments). It turned out to be a very good idea, because it streamlined DBTracker class: 1. Since DBTracker is not supposed to be called by public from now on, I renamed Open() to OpenDatabase() and made it an instance method. 2. I also changed return value in OpenDatabase() to TrackedDB*, because it was leveldb::DB* only for compatibility with DB::Open. 3. That simplified tests, because now I can check that TrackedDB::name() is correct right after OpenDatabase(), instead of inside VisitDatabases(). Additionally I made OpenDB() return unique_ptr instead of raw pointer. That's safer and inline with Chrome guidelines. Also, as can be seen in the follow-up CL (crrev.com/2953473002) all places in Chrome that call DB::Open are already storing returned DB pointers in unique_ptrs, so the change simplified code, too. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:240: class DBRegistry { On 2017/06/26 23:37:13, pwnall wrote: > On 2017/06/26 20:45:18, DmitrySkiba wrote: > > On 2017/06/22 02:50:22, pwnall wrote: > > > I'd prefer that Open() and VisitOpenDatabases() become functions in the > > > leveldb_env namespace, so calls look like leveldb_env::Open(). > > > leveldb_env::VisitOpenDatabases can proxy to the DBRegistry singleton > > instance, > > > which becomes an implementation detail. > > > > > > I prefer the name DBTracker to DBRegistry, because I think it conveys the > > > purpose better. Also, I'd prefer that Register / Unregister become > > > DatabaseOpened() / DatabaseClosed(), as this clarifies when they're supposed > > to > > > be called. > > > > Renaming: done. > > > > I'm not sure about free-standing functions. In my view the class serves as a > > clustering point for all related functionality. I.e. if you see that > > DBTracker::Open() is called, you know that there are probably more stuff in > > DBTracker. > > > > leveldb_env::Open() doesn't tell much - what is opened, and why not DB::Open? > > DBTracker::Open() provides more context. > > This comment (that I agree with) suggests that leveldb_env::OpenDatabase() / > OpenDB() would be better names than a naked Open(). I think that the context > given by DBTracker is unnecessary mental overhead for most readers, who just > want to get a leveldb::DB instance and get on with their lives. > > > Besides, the class provides convenient and unambiguous place to add new > > functionality. For example along with your comments I've added > > GetMemoryDumpName() API so that other MDPs could either provide additional > > per-database info, or reference database dumps. > > I'm not opposed to having the class for tracking-related functionality. Open / > OpenDatabase / OpenDB should definitely be exposed in the leveldb namespace, as > the tracking should be an implementation detail. VisitOpenDatabases can remain > in the tracker class, if that makes more sense. > > Also, in the interest of keeping CLs manageable in terms of size and review > time, please avoid adding more stuff after the review has started. Dependent CLs > FTW :) Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:254: virtual const std::string& name() const = 0; On 2017/06/26 23:37:13, pwnall wrote: > On 2017/06/26 20:45:19, DmitrySkiba wrote: > > On 2017/06/22 02:50:22, pwnall wrote: > > > Do you envision adding more functionality here, such as iterating over a > > > database's open iterators? > > > > > > If not, it might be simpler to add the name parameter to the DatabaseVisitor > > > function and remove this interface altogether. > > > > Yes, more stuff is planned: visiting iterators, exposing caches (similarly to > > https://chromium-review.googlesource.com/543439). > > How about TrackedDB instead of DB for the name, then? I'm not a big fan of empty > words like wrapper / proxy / decorator in APIs. The class comment can then state > that the class exposes the information tracked by DBTracker. Done. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:257: using DatabaseVisitor = std::function<void(DBWrapper*)>; On 2017/06/26 23:37:13, pwnall wrote: > On 2017/06/26 20:45:18, DmitrySkiba wrote: > > On 2017/06/22 02:50:22, pwnall wrote: > > > Visitor? > > > > I think that if method is called VisitDatabases(), then 'DatabaseVisitor' > makes > > more sense. > > I thought Visitor is acceptable if this is the only visitor, but I suppose we'll > have an iterator visitor as well at some point in the near future. Acknowledged. https://codereview.chromium.org/2855953002/diff/80001/third_party/leveldataba... third_party/leveldatabase/env_chromium.h:262: void VisitDatabases(const DatabaseVisitor& visitor); On 2017/06/26 23:37:13, pwnall wrote: > On 2017/06/26 20:45:19, DmitrySkiba wrote: > > On 2017/06/22 02:50:22, pwnall wrote: > > > VisitOpenDatabases? > > > > I feel that 'Open' is redundant, since it's the only option (i.e. we're not > > supporting visiting 'Closed' databases). > > It is, once you've thought things through. However, having it in is useful for a > reader that isn't fully familiar with the leveldb::DB lifecycle. For example, if > I forget how leveldb works, I'll wonder if it's possible to have DB instances > that have been created but not "opened" yet, and instances that have been closed > but not released, and I'll wonder what gets visited and what doesn't. > > I don't want to force readers to remember how LevelDB works to be able to reason > through this code. I think this is job for comments, so I updated the comment for VisitDatabases(). Now it mentions that 'live' databases are visited and gives exact criteria for 'liveness'. I think this makes things easier to understand - you open a database, and it's enumerated until its instance is destroyed. That would be the case even if there was Close() method in leveldb::DB. And BTW I was surprised to find out that in leveldb world 'closing a database' == 'destroying its instance' (I found out about that when I was writing tests) - i.e. I designed the API before that, without that specific leveldb knowledge. So the summary is that I don't think 'Open' makes sense there exactly because it's natural to expect something to be enumerated until it's dead, and the fact that in leveldb world 'closed' == 'dead' is an uncommon implementation detail. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:238: // Keeps track of live databases (opened via Open() method) and On 2017/06/26 23:37:14, pwnall wrote: > "Tracks database open via .... " seems shorter. > > IIUC, while this class exists to expose the information to memory-infra, it > doens't do the exposing directly, so I'd remove that sentence. As you mention in the next commend, GetMemoryDumpName() (which I'll re-introduce in the next CL) couples DBTracker to memory-infra. I.e. the main purpose of this class is indeed to expose databases to memory-infra. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:252: static std::string GetMemoryDumpName(leveldb::DB* db); On 2017/06/26 23:37:14, pwnall wrote: > FWIW, this method makes DBTracker depend on memory-infra, conceptually. Without > it, DBTracker is an abstract layer that helps inspect LevelDB. I don't know how > much you value this layering, but I figured it's worth noting. > > Separately, it might be worth noting if this only works correctly when used with > leveldb::DB instances produced by Open(). IIUC, that seems to be the case. > Assuming I'm correct, I think it'd also be worth adding a DCHECK verifying the > pre-condition. (the best solution I can think of is checking that the DB > instance is in the linked list) > > Given the extra complexity around this method, I think it'd make more sense to > introduce it in a separate CL that adds at least one MDP user, so it's easier to > see what the best design would be. I'll move the method to the next CL, and we can debate there. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:268: // databases themselves). Note that visiting order is not specified. On 2017/06/26 23:37:14, pwnall wrote: > "The databases may be visited in an arbitrary order." to also waive the > guarantee for a consistent order? Done. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... File third_party/leveldatabase/env_chromium_unittest.cc (right): https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium_unittest.cc:271: auto visit_databases = []() -> VisitedDBs { On 2017/06/26 23:37:14, pwnall wrote: > I'd prefer that you use a ChromiumEnvDBTrackerTest test fixture class and move > the types and VisitDatabases / AssertDatabasesLive there. Partially done. assert_databases_live() is still here, because it's used in one place and accesses local variable, but VisitDatabases() was moved to ChromiumEnvDBTrackerTest because it's now used in two tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
LGTM w/ nits. https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... File third_party/leveldatabase/env_chromium_unittest.cc (right): https://codereview.chromium.org/2855953002/diff/100001/third_party/leveldatab... third_party/leveldatabase/env_chromium_unittest.cc:271: auto visit_databases = []() -> VisitedDBs { On 2017/06/28 20:35:29, DmitrySkiba wrote: > On 2017/06/26 23:37:14, pwnall wrote: > > I'd prefer that you use a ChromiumEnvDBTrackerTest test fixture class and move > > the types and VisitDatabases / AssertDatabasesLive there. > > Partially done. assert_databases_live() is still here, because it's used in one > place and accesses local variable, but VisitDatabases() was moved to > ChromiumEnvDBTrackerTest because it's now used in two tests. I think this'd be more readable if the method would be AssertEqualSets(const VisitedDBSet&, const LiveDBSet&) where VisitedDBSet is a rename of VisitedDBs, and LiveDBSet is an alias for std::vector<std::unique_ptr<leveldb::DB>>. https://codereview.chromium.org/2855953002/diff/140001/third_party/leveldatab... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/140001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:252: // Opens a database and starts tracking it. As long as opened database nit: the opened database is alive https://codereview.chromium.org/2855953002/diff/140001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:261: // Calls |visitor| for each live database. Database is live from the point it Nit: The database is live
Description was changed from ========== 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 DBTracker::Open method, which is a drop-in replacement for DB::Open. Next CL will change all DB::Open invocations and add a presubmit check to prevent adding new ones. BUG=711518 ========== to ========== 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 ==========
The CQ bit was checked by dskiba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2855953002/#ps180001 (title: "Address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498768939561140,
"parent_rev": "60a4c98202b82c66d7418d2fc3bad1ff0d349726", "commit_rev":
"542a7c89b3e856ef642d3ff6352e661717172395"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/542a7c89b3e856ef642d3ff6352e... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/542a7c89b3e856ef642d3ff6352e...
Message was sent while issue was closed.
Sorry, I just realized this today. https://codereview.chromium.org/2855953002/diff/180001/third_party/leveldatab... File third_party/leveldatabase/env_chromium.h (right): https://codereview.chromium.org/2855953002/diff/180001/third_party/leveldatab... third_party/leveldatabase/env_chromium.h:259: using DatabaseVisitor = std::function<void(TrackedDB*)>; Gah, I just realized std::function is banned by the Chromium style guide. https://chromium-cpp.appspot.com/#library-blacklist Can you please prepare a CL that uses base::Callback instead? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
