Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310513004:
[tracing] Add memory statistics from level db clients to tracing. (Closed)
|
Created:
5 years, 3 months ago by ssid Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@skia_res Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Add memory statistics from level db clients to tracing.
The major clients of level database are leveldb value store
(just a wrapper around leveldb). This CL adds memory
statistics of the opened databases to tracing.
BUG=547066
Committed: https://crrev.com/736929d3c3722ea3d9e4d3b9493e99c003d3d712
Cr-Commit-Position: refs/heads/master@{#358303}
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 12
Patch Set 3 : Thread safety and naming. #
Total comments: 4
Patch Set 4 : Fix double lock. #
Total comments: 11
Patch Set 5 : Remove session database. #Patch Set 6 : Nit. #Patch Set 7 : Fix mac suballocation and make names unique. #
Messages
Total messages: 32 (13 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
Now that the level db changes have rolled, the new api will be available in chromium ( https://codereview.chromium.org/1395873003/). PTAL, thanks.
How many instances of these dump providers do you expect to be around? https://codereview.chromium.org/1310513004/diff/20001/content/browser/dom_sto... File content/browser/dom_storage/session_storage_database.cc (right): https://codereview.chromium.org/1310513004/diff/20001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:106: this); if you unregister the RegisterDumpProvider call above must specify a task runner. Otherwise you can unregister while dumping on another thread, which is racy. There is a dcheck preventing that in UnregisterDumpProvider, did you not hit this in debug builds? https://codereview.chromium.org/1310513004/diff/20001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:344: db_->GetProperty("leveldb.approximate-memory-usage", &value); shouldn't you check the return value of GetProperty? https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... File extensions/browser/value_store/leveldb_value_store.cc (right): https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:65: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( same here about threading and unregistration https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:71: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( And I guess here you meant to UNregister, not re-register https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:338: if (!db_) you really need to think about threading here. What is the thread on which db_ lives, I think FILE thread? You can't ask to dump on an arbitrary thread and then just access db_ https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:347: base::StringPrintf("leveldb/value_store/%p", this)); why not putting the uma_client_name or the basename(db_path) in the name?
Patchset #3 (id:40001) has been deleted
I have not seen more than 3-4 instances of the database. https://codereview.chromium.org/1310513004/diff/20001/content/browser/dom_sto... File content/browser/dom_storage/session_storage_database.cc (right): https://codereview.chromium.org/1310513004/diff/20001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:106: this); On 2015/10/15 08:59:52, Primiano Tucci wrote: > if you unregister the RegisterDumpProvider call above must specify a task > runner. Otherwise you can unregister while dumping on another thread, which is > racy. > There is a dcheck preventing that in UnregisterDumpProvider, did you not hit > this in debug builds? Thanks, I guess both were done in the same thread and i did not hit DCHECK. https://codereview.chromium.org/1310513004/diff/20001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:344: db_->GetProperty("leveldb.approximate-memory-usage", &value); On 2015/10/15 08:59:52, Primiano Tucci wrote: > shouldn't you check the return value of GetProperty? Actually it cannot fail. added dcheck. https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... File extensions/browser/value_store/leveldb_value_store.cc (right): https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:65: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( On 2015/10/15 08:59:53, Primiano Tucci wrote: > same here about threading and unregistration Done. https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:71: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( On 2015/10/15 08:59:53, Primiano Tucci wrote: > And I guess here you meant to UNregister, not re-register Done. https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:338: if (!db_) On 2015/10/15 08:59:53, Primiano Tucci wrote: > you really need to think about threading here. What is the thread on which db_ > lives, I think FILE thread? You can't ask to dump on an arbitrary thread and > then just access db_ Done. https://codereview.chromium.org/1310513004/diff/20001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:347: base::StringPrintf("leveldb/value_store/%p", this)); On 2015/10/15 08:59:53, Primiano Tucci wrote: > why not putting the uma_client_name or the basename(db_path) in the name? Done.
ok, LGTM from the memory-infra viewpoint (% double lock). Loop in the owners here and see what they think? https://codereview.chromium.org/1310513004/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/session_storage_database.cc (right): https://codereview.chromium.org/1310513004/diff/60001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:342: why are you locking db_lock_ twice? doesn't this deadlock? Also, do you really need to take the lock at all here? Plz discuss this with the owners https://codereview.chromium.org/1310513004/diff/60001/extensions/browser/valu... File extensions/browser/value_store/leveldb_value_store.cc (right): https://codereview.chromium.org/1310513004/diff/60001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:350: base::ReplaceChars(open_histogram_->histogram_name(), ".", "_", &name); this should not needed anymore, I removed the awkward bug about '.' a while ago
https://codereview.chromium.org/1310513004/diff/60001/content/browser/dom_sto... File content/browser/dom_storage/session_storage_database.cc (right): https://codereview.chromium.org/1310513004/diff/60001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:342: On 2015/10/15 15:27:56, Primiano Tucci wrote: > why are you locking db_lock_ twice? doesn't this deadlock? > Also, do you really need to take the lock at all here? Plz discuss this with the > owners yes removed. https://codereview.chromium.org/1310513004/diff/60001/extensions/browser/valu... File extensions/browser/value_store/leveldb_value_store.cc (right): https://codereview.chromium.org/1310513004/diff/60001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:350: base::ReplaceChars(open_histogram_->histogram_name(), ".", "_", &name); On 2015/10/15 15:27:56, Primiano Tucci wrote: > this should not needed anymore, I removed the awkward bug about '.' a while ago Done.
ssid@chromium.org changed reviewers: + marja@chromium.org, michaeln@chromium.org, reillyg@chromium.org
This CL adds memory statistics of the databases to tracing and does not change actual functionality. reillyg@ for changes in dom_storage/. Could you please confirm if I need the to take the lock in SessionStorage database? michaeln@ and marja@ for changes in value_store/. PTAL, thanks.
reillyg@chromium.org changed reviewers: + cmumford@chromium.org
cmumford@, can you take a look at //extensions/browser/value_store? I'm not familiar with this code.
I'm not an owner of //extensions/browser/value_store - just minor comments/questions from me. Generally looking good to me. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... File extensions/browser/value_store/leveldb_value_store.cc (right): https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:341: return true; Should you be returning false here? https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:346: DCHECK(res); What about returning false if GetProperty returns false? I'm OK with the DCHECK - because we really know that leveldb will always succeed (assuming it's the right version), but it'd be more future-proof to leveldb changes if we add this check. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:347: base::StringToUint64(value, &size); I guess not super important to check this, but might not be a bad idea. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:349: base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump( You could make this "auto" if you want - but perfectly fine as-is.
michaeln@chromium.org changed reviewers: - marja@chromium.org
-marja (she doesn't work on this anymore, i'll add a comment to the owners file) https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/session_storage_database.cc (right): https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:102: this, base::ThreadTaskRunnerHandle::Get()); The threading needs some work. This class is created on the main thread and deleted on a worker pool thread. DCHECK(!base::ThreadTaskRunnerHandle::IsSet()) is a valid assertion on worker pool threads. Does the MemoryDumpManager know about SequencedTaskRunners? Session storage doesn't care about thread affinity but it does care about ordering. It uses the SequencedWorkerPool and SequencedTaskRunners for that. A lot of things care about ordering more than thread affinity so modifying the dump manager to work with them instead of ThreadTaskRunners would be a good change. https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:359: bool SessionStorageDatabase::LazyOpen(bool create_if_needed) { This might be a better place to Register with the dumpmgr but we need to make the task runner available to this class.
https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... File content/browser/dom_storage/session_storage_database.cc (right): https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... content/browser/dom_storage/session_storage_database.cc:102: this, base::ThreadTaskRunnerHandle::Get()); On 2015/10/15 23:55:13, michaeln wrote: > The threading needs some work. This class is created on the main thread and > deleted on a worker pool thread. > > DCHECK(!base::ThreadTaskRunnerHandle::IsSet()) is a valid assertion on worker > pool threads. > > Does the MemoryDumpManager know about SequencedTaskRunners? Session storage > doesn't care about thread affinity but it does care about ordering. It uses the > SequencedWorkerPool and SequencedTaskRunners for that. A lot of things care > about ordering more than thread affinity so modifying the dump manager to work > with them instead of ThreadTaskRunners would be a good change. > There are mainly two reason why I restricted the manager to ThreadTaskRunners : - I want to guarantee that Registration, Unregistration and OnMemoryDump calls do not overlap. Restricting to a single thread felt the safe choice to me. But is sounds like we might be able to get the same guarantee with STR, but I have to think more on this. - By using thread task runners I can make the manager smarter on the thread hopping logic: if I have 20 dumpers registered on 5 different threads, I can reshuffle the OnMemoryDump sequence grouping by thread and do only 5 thread hops in total. If I switch to STR, how do I figure out what is the smartest sequence of hopping? I guess the best choice would be to have a different code path for STR, and just do uncoditional PostTasks all the times there.
On 2015/10/16 09:10:34, Primiano Tucci wrote: > https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... > File content/browser/dom_storage/session_storage_database.cc (right): > > https://codereview.chromium.org/1310513004/diff/80001/content/browser/dom_sto... > content/browser/dom_storage/session_storage_database.cc:102: this, > base::ThreadTaskRunnerHandle::Get()); > On 2015/10/15 23:55:13, michaeln wrote: > > The threading needs some work. This class is created on the main thread and > > deleted on a worker pool thread. > > > > DCHECK(!base::ThreadTaskRunnerHandle::IsSet()) is a valid assertion on worker > > pool threads. > > > > Does the MemoryDumpManager know about SequencedTaskRunners? Session storage > > doesn't care about thread affinity but it does care about ordering. It uses > the > > SequencedWorkerPool and SequencedTaskRunners for that. A lot of things care > > about ordering more than thread affinity so modifying the dump manager to work > > with them instead of ThreadTaskRunners would be a good change. > > > > There are mainly two reason why I restricted the manager to ThreadTaskRunners : > - I want to guarantee that Registration, Unregistration and OnMemoryDump calls > do not overlap. Restricting to a single thread felt the safe choice to me. But > is sounds like we might be able to get the same guarantee with STR, but I have > to think more on this. SequencedTaskRunners guarantee non-concurrent serialization. I think thats what you want. > - By using thread task runners I can make the manager smarter on the thread > hopping logic: if I have 20 dumpers registered on 5 different threads, I can > reshuffle the OnMemoryDump sequence grouping by thread and do only 5 thread hops > in total. > > If I switch to STR, how do I figure out what is the smartest sequence of > hopping? > I guess the best choice would be to have a different code path for STR, and just > do uncoditional PostTasks all the times there. Comparing taskrunner ptrs is probably the best you could do with SequencedTaskRunners.
Description was changed from ========== [tracing] Add memory statistics from level db clients to tracing. The major clients of level database are session storage and leveldb value store (just a wrapper around leveldb). This CL adds memory statistics of the opened databases to tracing. BUG=466141 ========== to ========== [tracing] Add memory statistics from level db clients to tracing. The major clients of level database are session storage and leveldb value store (just a wrapper around leveldb). This CL adds memory statistics of the opened databases to tracing. BUG=547066 ==========
cmumford@ and reillyg@ could you please take another look at this CL, for value store? michaeln@ I have removed the session storage database from this CL. This is mainly because of the issue with sequenced task runners. primiano@ is working on supporting the sequenced task runner / thread safe dumping. Mean while, the value store databases are always working on FILE thread and can be added to tracing. Thanks. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... File extensions/browser/value_store/leveldb_value_store.cc (right): https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:341: return true; On 2015/10/15 18:35:44, cmumford wrote: > Should you be returning false here? This has to return true for temporary failures and return false only for permanent failures. If it returns false the provider is disabled forever. So,returning true. I guess this value must be changed to an enum. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:346: DCHECK(res); On 2015/10/15 18:35:44, cmumford wrote: > What about returning false if GetProperty returns false? I'm OK with the DCHECK > - because we really know that leveldb will always succeed (assuming it's the > right version), but it'd be more future-proof to leveldb changes if we add this > check. The DCHECK is placed so that if in future the api changes the build breaks and we will fix it. Instead of silently ignoring the error. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:347: base::StringToUint64(value, &size); On 2015/10/15 18:35:44, cmumford wrote: > I guess not super important to check this, but might not be a bad idea. Done. https://codereview.chromium.org/1310513004/diff/80001/extensions/browser/valu... extensions/browser/value_store/leveldb_value_store.cc:349: base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump( On 2015/10/15 18:35:44, cmumford wrote: > You could make this "auto" if you want - but perfectly fine as-is. Done.
lgtm
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310513004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310513004/120001
Description was changed from ========== [tracing] Add memory statistics from level db clients to tracing. The major clients of level database are session storage and leveldb value store (just a wrapper around leveldb). This CL adds memory statistics of the opened databases to tracing. BUG=547066 ========== to ========== [tracing] Add memory statistics from level db clients to tracing. The major clients of level database are leveldb value store (just a wrapper around leveldb). This CL adds memory statistics of the opened databases to tracing. BUG=547066 ==========
Description was changed from ========== [tracing] Add memory statistics from level db clients to tracing. The major clients of level database are leveldb value store (just a wrapper around leveldb). This CL adds memory statistics of the opened databases to tracing. BUG=547066 ========== to ========== [tracing] Add memory statistics from level db clients to tracing. The major clients of level database are leveldb value store (just a wrapper around leveldb). This CL adds memory statistics of the opened databases to tracing. BUG=547066 ==========
Memory-infra side LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/1310513004/#ps160001 (title: "Fix mac suballocation and make names unique.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310513004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310513004/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/736929d3c3722ea3d9e4d3b9493e99c003d3d712 Cr-Commit-Position: refs/heads/master@{#358303} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
