Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1465623002:
ipc: Add basic memory usage tests for attachment brokering. (Closed)
Descriptionipc: Add basic memory usage tests for attachment brokering.
I added two tests for memory leaks in the attachment brokering process. The
first checks memory usage after brokering a single, large, shared memory object.
The second checks memory usage after brokering a thousand, smaller memory
objects.
BUG=557387
Committed: https://crrev.com/a519e56d03e1f6d69b7475bdd006f5e47b4f581e
Cr-Commit-Position: refs/heads/master@{#360935}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Comments from mark. #
Total comments: 2
Patch Set 3 : Comments from tsepez. #Patch Set 4 : Comments from tsepez. #Messages
Total messages: 18 (5 generated)
erikchen@chromium.org changed reviewers: + tsepez@chromium.org
tsepez: Please review.
https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... File ipc/attachment_broker_mac_unittest.cc (right): https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:1205: EXPECT_LE(GetResidentSize(), globals->initial_resident_size + message_size); I worry about these kinds of tests being flakey based upon the environment etc. You'll need someone with more mac internals to also take a look at this.
erikchen@chromium.org changed reviewers: + mark@chromium.org
mark: Please review.
Seems like it might be flaky, but if it’s in fact not flaky practically speaking, I’ll say LGTM to it. Maybe leave a comment behind saying that it might be flaky, though, so that if it does become flaky under some future compiler or test architecture change, the person dealing with it will know about our confidence level. Better yet, so that the person whose unrelated change is blamed for the test failure will have a handy scapegoat. https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... File ipc/attachment_broker_mac_unittest.cc (right): https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:50: LOG(FATAL) << "Couldn't get resident size."; Basically just CHECK(kr == KERN_SUCCESS), or even MACH_CHECK(kr == KERN_SUCCESS, kr), then. https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:1204: size_t message_size = 1024 * 1024 * 8; Share with message_size in the TEST_F above? Same for the many-messages test. https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:1205: EXPECT_LE(GetResidentSize(), globals->initial_resident_size + message_size); I don’t know if we can really strictly say that there’ll be an LE relationship here.
tsepez: PTAL https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... File ipc/attachment_broker_mac_unittest.cc (right): https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:50: LOG(FATAL) << "Couldn't get resident size."; On 2015/11/19 22:54:34, Mark Mentovai wrote: > Basically just CHECK(kr == KERN_SUCCESS), or even MACH_CHECK(kr == KERN_SUCCESS, > kr), then. Done. https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:1204: size_t message_size = 1024 * 1024 * 8; On 2015/11/19 22:54:34, Mark Mentovai wrote: > Share with message_size in the TEST_F above? Same for the many-messages test. Done. https://codereview.chromium.org/1465623002/diff/1/ipc/attachment_broker_mac_u... ipc/attachment_broker_mac_unittest.cc:1205: EXPECT_LE(GetResidentSize(), globals->initial_resident_size + message_size); On 2015/11/19 22:54:34, Mark Mentovai wrote: > I don’t know if we can really strictly say that there’ll be an LE relationship > here. you're right, I was using message_size for two different purposes. Fixed.
https://codereview.chromium.org/1465623002/diff/20001/ipc/attachment_broker_m... File ipc/attachment_broker_mac_unittest.cc (right): https://codereview.chromium.org/1465623002/diff/20001/ipc/attachment_broker_m... ipc/attachment_broker_mac_unittest.cc:1223: volatile char c = '\0'; I suspect that an aggressive optimizer may be able to pull this anyhow, since it can prove that c is unused, and that its address can't be aliased with any other pointers, etc. I think you want static_cast<volatile char*>(addr) which tells the compiler that the addr changes unpredictably and that the act of reading it may have some side-effect (like auto-incrementing device registers should they be mapped there).
https://codereview.chromium.org/1465623002/diff/20001/ipc/attachment_broker_m... File ipc/attachment_broker_mac_unittest.cc (right): https://codereview.chromium.org/1465623002/diff/20001/ipc/attachment_broker_m... ipc/attachment_broker_mac_unittest.cc:1223: volatile char c = '\0'; On 2015/11/20 17:23:03, Tom Sepez wrote: > I suspect that an aggressive optimizer may be able to pull this anyhow, since it > can prove that c is unused, and that its address can't be aliased with any other > pointers, etc. > > I think you want static_cast<volatile char*>(addr) which tells the compiler that > the addr changes unpredictably and that the act of reading it may have some > side-effect (like auto-incrementing device registers should they be mapped > there). I spent some time thinking about this, and decided that your suggestion was correct, and that my previous code was incorrect.
lgtm
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/1465623002/#ps60001 (title: "Comments from tsepez.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465623002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1465623002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a519e56d03e1f6d69b7475bdd006f5e47b4f581e Cr-Commit-Position: refs/heads/master@{#360935}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1466633003/ by dcheng@chromium.org. The reason for reverting is: Failing on Mac ASan 64 Tests (1).
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests... Example test failure: IPCAttachmentBrokerMacTest.MemoryUsageManyMessages (run #1): [ RUN ] IPCAttachmentBrokerMacTest.MemoryUsageManyMessages [99259:3847:1120/182039:22097145484234:WARNING:test_suite.cc(197)] Test launcher output path /var/folders/65/nfq1c1v9661b310zc_n399tr0000gp/T/.org.chromium.Chromium.7kx3ef/test_results.xml exists. Not adding test launcher result printer. [99259:3847:1120/182039:22097145638327:INFO:attachment_broker_mac_unittest.cc(551)] Privileged process start. [99259:3847:1120/182039:22097146340818:INFO:attachment_broker_mac_unittest.cc(568)] Privileged process spinning run loop. [99259:3847:1120/182039:22097148139567:INFO:attachment_broker_mac_unittest.cc(576)] Privileged process running callback. [99259:3847:1120/182039:22097148480032:INFO:attachment_broker_mac_unittest.cc(1300)] Disable privileged process message logging. [99259:3847:1120/182039:22097963456543:INFO:attachment_broker_mac_unittest.cc(1307)] Increase in memory usage in KB: 9464 ../../ipc/attachment_broker_mac_unittest.cc:439: Failure Value of: RESULT_SUCCESS Actual: 1 Expected: get_result_listener()->get_result() Which is: 2 [99259:3847:1120/182039:22097964293731:INFO:attachment_broker_mac_unittest.cc(587)] Privileged process end. [ FAILED ] IPCAttachmentBrokerMacTest.MemoryUsageManyMessages (1733 ms) |
