Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541073003:
Instrument SdchManager using MemoryDumpProvider (Closed)
DescriptionInstrument SdchManager using MemoryDumpProvider
This CL instruments SdchManager using MemoryDumpProvider.
Even though SDCH might be unshipped (see blink-dev),
it is useful to get an idea of how much memory dictionaries
cost.
BUG=669108
Committed: https://crrev.com/d701b6e5acd6f6f5c01224ebb1c42844773d7be2
Cr-Commit-Position: refs/heads/master@{#441485}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address ssid@ comment #
Total comments: 6
Patch Set 3 : Address Randy's comments #
Depends on Patchset: Messages
Total messages: 26 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Instrument SdchManager using MemoryDumpProvider This CL instruments SdchManager using MemoryDumpProvider. BUG=669108 ========== to ========== Instrument SdchManager using MemoryDumpProvider This CL instruments SdchManager using MemoryDumpProvider. Even though SDCH might be unshipped (see blink-dev), it is useful to get an idea of how much memory dictionaries cost. BUG=669108 ==========
xunjieli@chromium.org changed reviewers: + rdsmith@chromium.org
Patchset #1 (id:60001) has been deleted
Randy, PTAL. Thank you!
The CQ bit was checked by xunjieli@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...
Here's a sample trace: https://drive.google.com/a/google.com/file/d/0B_9WseIqRoaFcEFDODM1SENfOGc/vie... which shows a SdchManager shared between Main URLRequestContext and Main Media URLRequestContext. The dictionaries are ~1.5MiB in size.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ssid@chromium.org changed reviewers: + ssid@chromium.org
https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.c... net/base/sdch_manager.cc:340: return; I feel if you could move this to the top of the function, it is easier to understand what is going on. It took a moment to understand when the dump is created. https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.c... net/base/sdch_manager.cc:355: base::trace_event::MemoryAllocatorDump* empty_row_dump = I really wish there was a way to add size to this dump rather than the "net/sdch_manager" dump. Then we would clearly see how much usage is caused by media or safe_browsing. There's nothing else you could do here though. https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.c... net/base/sdch_manager.cc:357: "%s/sdch_manager", parent_dump_absolute_name.c_str())); You could just do (parent_dump_absolute_name + "/sdch_manager")
Thanks! PTAL. https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.c... net/base/sdch_manager.cc:340: return; On 2016/12/15 23:00:32, ssid wrote: > I feel if you could move this to the top of the function, it is easier to > understand what is going on. It took a moment to understand when the dump is > created. Done. https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.c... net/base/sdch_manager.cc:355: base::trace_event::MemoryAllocatorDump* empty_row_dump = On 2016/12/15 23:00:32, ssid wrote: > I really wish there was a way to add size to this dump rather than the > "net/sdch_manager" dump. Then we would clearly see how much usage is caused by > media or safe_browsing. > There's nothing else you could do here though. Acknowledged. https://codereview.chromium.org/2541073003/diff/80001/net/base/sdch_manager.c... net/base/sdch_manager.cc:357: "%s/sdch_manager", parent_dump_absolute_name.c_str())); On 2016/12/15 23:00:32, ssid wrote: > You could just do (parent_dump_absolute_name + "/sdch_manager") Done.
First of all, I'm sorry I didn't respond to this review before this point--somehow I didn't realize how long it had been lying fallow. I've just reviewed the net/ changes, specifically the SdchManager access and correctly getting the total size used by SDCH. For those parts of it, LGTM, though I'll note that I'd like include file change and memory size test, but I don't want to be in your way and I trust your judgement if you think there's something I've missed that make those bad ideas. https://codereview.chromium.org/2541073003/diff/100001/net/base/sdch_manager_... File net/base/sdch_manager_unittest.cc (right): https://codereview.chromium.org/2541073003/diff/100001/net/base/sdch_manager_... net/base/sdch_manager_unittest.cc:682: ASSERT_EQ("1", count); Why not EXPECT_EQ for this one? I think we want the observer to be removed even if the counts off. https://codereview.chromium.org/2541073003/diff/100001/net/base/sdch_manager_... net/base/sdch_manager_unittest.cc:682: ASSERT_EQ("1", count); I'd feel better if there was some test that compared the amount of memory charged against the manager with the size of dictionary_text. Is there a reason not to do that? https://codereview.chromium.org/2541073003/diff/100001/net/url_request/url_re... File net/url_request/url_request_context.h (right): https://codereview.chromium.org/2541073003/diff/100001/net/url_request/url_re... net/url_request/url_request_context.h:22: #include "net/base/sdch_manager.h" Why? My read of url_request_context.h is that it only refers to SdchManager by pointer, so a forward decl should be fine.
Thanks a lot for the review! This one is blocked on another CL (https://codereview.chromium.org/2560593002). I will wait after the holidays to ping my reviewers on that one, then I will land this one. Have a great holiday! :) https://codereview.chromium.org/2541073003/diff/100001/net/base/sdch_manager_... File net/base/sdch_manager_unittest.cc (right): https://codereview.chromium.org/2541073003/diff/100001/net/base/sdch_manager_... net/base/sdch_manager_unittest.cc:682: ASSERT_EQ("1", count); On 2016/12/21 20:21:38, Randy Smith - Not in Mondays wrote: > Why not EXPECT_EQ for this one? I think we want the observer to be removed even > if the counts off. Done. https://codereview.chromium.org/2541073003/diff/100001/net/base/sdch_manager_... net/base/sdch_manager_unittest.cc:682: ASSERT_EQ("1", count); On 2016/12/21 20:21:38, Randy Smith - Not in Mondays wrote: > I'd feel better if there was some test that compared the amount of memory > charged against the manager with the size of dictionary_text. Is there a reason > not to do that? Done. https://codereview.chromium.org/2541073003/diff/100001/net/url_request/url_re... File net/url_request/url_request_context.h (right): https://codereview.chromium.org/2541073003/diff/100001/net/url_request/url_re... net/url_request/url_request_context.h:22: #include "net/base/sdch_manager.h" On 2016/12/21 20:21:38, Randy Smith - Not in Mondays wrote: > Why? My read of url_request_context.h is that it only refers to SdchManager by > pointer, so a forward decl should be fine. Done. Sorry, I meant to add it to the cc file. Thanks for catching this!
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2541073003/#ps120001 (title: "Address Randy's 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": 120001, "attempt_start_ts": 1483564764126730,
"parent_rev": "446fc5ecb65fe841c9e3f825dfc517a611db63c3", "commit_rev":
"33c2649a8483f2a10bb2729604303632332ce08c"}
Message was sent while issue was closed.
Description was changed from ========== Instrument SdchManager using MemoryDumpProvider This CL instruments SdchManager using MemoryDumpProvider. Even though SDCH might be unshipped (see blink-dev), it is useful to get an idea of how much memory dictionaries cost. BUG=669108 ========== to ========== Instrument SdchManager using MemoryDumpProvider This CL instruments SdchManager using MemoryDumpProvider. Even though SDCH might be unshipped (see blink-dev), it is useful to get an idea of how much memory dictionaries cost. BUG=669108 Review-Url: https://codereview.chromium.org/2541073003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Instrument SdchManager using MemoryDumpProvider This CL instruments SdchManager using MemoryDumpProvider. Even though SDCH might be unshipped (see blink-dev), it is useful to get an idea of how much memory dictionaries cost. BUG=669108 Review-Url: https://codereview.chromium.org/2541073003 ========== to ========== Instrument SdchManager using MemoryDumpProvider This CL instruments SdchManager using MemoryDumpProvider. Even though SDCH might be unshipped (see blink-dev), it is useful to get an idea of how much memory dictionaries cost. BUG=669108 Committed: https://crrev.com/d701b6e5acd6f6f5c01224ebb1c42844773d7be2 Cr-Commit-Position: refs/heads/master@{#441485} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d701b6e5acd6f6f5c01224ebb1c42844773d7be2 Cr-Commit-Position: refs/heads/master@{#441485} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
