Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2340983002:
[tracing] Memory dump provider for leveldb_proto (Closed)
Description[tracing] Memory dump provider for leveldb_proto
This CL makes leveldb_proto a memory dump provider for adding statistics
to chrome://tracing. This would cover ntp snippet, thumbnail and image
databases.
BUG=645126
Committed: https://crrev.com/e4aba36676c616489620442d8ae6aa88931e680c
Cr-Commit-Position: refs/heads/master@{#420997}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add dcheck #Patch Set 3 : fix tests. #
Total comments: 10
Patch Set 4 : Fixes. #
Total comments: 2
Patch Set 5 : Fix test name and IOS build. #
Messages
Total messages: 49 (35 generated)
ssid@chromium.org changed reviewers: + dskiba@chromium.org
But, I am not able to hit the case where chrome creates databases. Maybe I have some login issue or different account. Could you please patch this CL and check if this number matches the size shown in heap profiler?
https://codereview.chromium.org/2340983002/diff/1/components/leveldb_proto/le... File components/leveldb_proto/leveldb_database.cc (right): https://codereview.chromium.org/2340983002/diff/1/components/leveldb_proto/le... components/leveldb_proto/leveldb_database.cc:180: dump->AddString("name", "", open_histogram_->histogram_name()); This causes name to have "LevelDB.Open." prefix, for example LevelDB.Open.ImageManager. Just ImageManager would be clearer.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
https://codereview.chromium.org/2340983002/diff/1/components/leveldb_proto/le... File components/leveldb_proto/leveldb_database.cc (right): https://codereview.chromium.org/2340983002/diff/1/components/leveldb_proto/le... components/leveldb_proto/leveldb_database.cc:180: dump->AddString("name", "", open_histogram_->histogram_name()); On 2016/09/15 17:15:59, DmitrySkiba wrote: > This causes name to have "LevelDB.Open." prefix, for example > LevelDB.Open.ImageManager. Just ImageManager would be clearer. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
ssid@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist ptal, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... File components/leveldb_proto/leveldb_database.cc (right): https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:53: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( is thiss safe to do even if Register was never called? https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:163: base::trace_event::ProcessMemoryDump* pmd) { Is there any way of testing this? https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:167: std::string value; Nit: Could you add an empty line before this? https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:174: auto* dump = pmd->CreateAllocatorDump( Could this just be base::MemoryAllocatorDump*? I don't find that auto increases readability in this case. https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... File components/leveldb_proto/leveldb_database.h (right): https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.h:63: std::string client_name_; Could you add a comment as to what this is used for?
https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... File components/leveldb_proto/leveldb_database.cc (right): https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:53: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( On 2016/09/21 06:04:16, nyquist wrote: > is thiss safe to do even if Register was never called? yes. https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:163: base::trace_event::ProcessMemoryDump* pmd) { On 2016/09/21 06:04:16, nyquist wrote: > Is there any way of testing this? added unittest https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:167: std::string value; On 2016/09/21 06:04:16, nyquist wrote: > Nit: Could you add an empty line before this? Done. https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.cc:174: auto* dump = pmd->CreateAllocatorDump( On 2016/09/21 06:04:16, nyquist wrote: > Could this just be base::MemoryAllocatorDump*? I don't find that auto increases > readability in this case. auto bcos base::trace_event::MemoryAllocatorDump was too long. Fixed. https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... File components/leveldb_proto/leveldb_database.h (right): https://codereview.chromium.org/2340983002/diff/80001/components/leveldb_prot... components/leveldb_proto/leveldb_database.h:63: std::string client_name_; On 2016/09/21 06:04:16, nyquist wrote: > Could you add a comment as to what this is used for? Done.
Patchset #5 (id:100001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
lgtm https://codereview.chromium.org/2340983002/diff/160001/components/leveldb_pro... File components/leveldb_proto/proto_database_impl_unittest.cc (right): https://codereview.chromium.org/2340983002/diff/160001/components/leveldb_pro... components/leveldb_proto/proto_database_impl_unittest.cc:577: TEST(ProtoDatabaseImplLevelDBTest, TestOnMemoryDump) { TestOnMemoryDumpEmitsData
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Patchset #5 (id:220001) has been deleted
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: This issue passed the CQ dry run.
Patchset #5 (id:240001) has been deleted
Patchset #5 (id:260001) has been deleted
Also fixed ios build. https://codereview.chromium.org/2340983002/diff/160001/components/leveldb_pro... File components/leveldb_proto/proto_database_impl_unittest.cc (right): https://codereview.chromium.org/2340983002/diff/160001/components/leveldb_pro... components/leveldb_proto/proto_database_impl_unittest.cc:577: TEST(ProtoDatabaseImplLevelDBTest, TestOnMemoryDump) { On 2016/09/23 15:57:32, nyquist wrote: > TestOnMemoryDumpEmitsData Done.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dskiba@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2340983002/#ps280001 (title: "Fix test name and IOS build.")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Memory dump provider for leveldb_proto This CL makes leveldb_proto a memory dump provider for adding statistics to chrome://tracing. This would cover ntp snippet, thumbnail and image databases. BUG=645126 ========== to ========== [tracing] Memory dump provider for leveldb_proto This CL makes leveldb_proto a memory dump provider for adding statistics to chrome://tracing. This would cover ntp snippet, thumbnail and image databases. BUG=645126 Committed: https://crrev.com/e4aba36676c616489620442d8ae6aa88931e680c Cr-Commit-Position: refs/heads/master@{#420997} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e4aba36676c616489620442d8ae6aa88931e680c Cr-Commit-Position: refs/heads/master@{#420997} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
