Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648423006:
Fix ExhaustMemory on clang. (Closed)
|
Created:
3 years, 11 months ago by Will Harris Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, vmpstr+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ExhaustMemory on clang.
Also, bolster the current tests to verify sad tab behavior.
BUG=681218
Review-Url: https://codereview.chromium.org/2648423006
Cr-Commit-Position: refs/heads/master@{#450078}
Committed: https://chromium.googlesource.com/chromium/src/+/3dcb0a73e6f194be62930e7e1924a0b6965f5637
Patch Set 1 #Patch Set 2 : unused function #Patch Set 3 : all arch #
Total comments: 2
Patch Set 4 : move test to browser tests and rebase #Patch Set 5 : fixes to tests #
Total comments: 10
Patch Set 6 : address code review comments #Patch Set 7 : add comment, fix issue #
Total comments: 5
Patch Set 8 : final nits #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== Fix ExhaustMemory on clang. BUG=681218 ========== to ========== Fix ExhaustMemory on clang. BUG=681218 ==========
wfh@chromium.org changed reviewers: + thakis@chromium.org
No reason to believe this won't work, but I'm a little wary of doing that kind of memory exhaust on the bots, in 64-bit. WDYT?
On 2017/01/25 22:36:52, Will Harris wrote: > No reason to believe this won't work, but I'm a little wary of doing that kind > of memory exhaust on the bots, in 64-bit. WDYT? Although, the test didn't take long on 64-bit so something must be failing fast [699/2318] OutOfMemoryDeathTest.Exhaust (96 ms) <-- [700/2318] OutOfMemoryDeathTest.New (59 ms) [701/2318] OutOfMemoryDeathTest.NewArray (50 ms) [702/2318] OutOfMemoryDeathTest.Malloc (51 ms) [703/2318] OutOfMemoryDeathTest.Realloc (50 ms) [704/2318] OutOfMemoryDeathTest.Calloc (55 ms) [705/2318] OutOfMemoryDeathTest.AlignedAlloc (62 ms)
https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... base/process/memory_unittest.cc:151: } do we really need this test? it duplicates the function under test and it adds a death test, which both seems a bit suboptimal. i'd either revert changes to this file, or make it so that the old cold calls the function that's actually being tested here.
https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... File base/process/memory_unittest.cc (right): https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... base/process/memory_unittest.cc:151: } On 2017/01/26 17:56:53, Nico wrote: > do we really need this test? it duplicates the function under test and it adds a > death test, which both seems a bit suboptimal. i'd either revert changes to this > file, or make it so that the old cold calls the function that's actually being > tested here. I'm not sure I totally understand what you mean by "duplicates the function under test", but alternatively I could move the test to a browser test and navigate to the URL instead, would that suffice? Historically we've been okay with death tests for the memory code (see the rest of the file), but I agree it's non-optimal to duplicate the ExhaustMemory code in two places.
On 2017/01/26 18:12:44, Will Harris wrote: > https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... > File base/process/memory_unittest.cc (right): > > https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... > base/process/memory_unittest.cc:151: } > On 2017/01/26 17:56:53, Nico wrote: > > do we really need this test? it duplicates the function under test and it adds > a > > death test, which both seems a bit suboptimal. i'd either revert changes to > this > > file, or make it so that the old cold calls the function that's actually being > > tested here. > > I'm not sure I totally understand what you mean by "duplicates the function > under test" It copies ExhaustMemory() from render_frame_impl.cc to memory_unittest.cc >, but alternatively I could move the test to a browser test and > navigate to the URL instead, would that suffice? It'd take 30x longer to run and probably be flaky, but that'd work for me if you prefer that. > Historically we've been okay with death tests for the memory code (see the rest > of the file), but I agree it's non-optimal to duplicate the ExhaustMemory code > in two places. I guess in my mind I was scoring the test bit as "test coverage: +10 points, duplicates code under test -10 points, death test -1 point – oh score for test is below 0, better ask if we really want this test". So any way of not duping the function makes the test worthwhile. Options: - put function in render_frame_impl header, put test in new render_frame_impl_unittest.cc (and ref it from content/test/BUILD.gn) … probably best option - put function in base, keep test where it is, make render_frame_impl.cc call that. con: new one-time-use function in base - write browser test instead. pro: integration test for what we're shipping. con: takes much longer to run and is likely more flaky
Description was changed from ========== Fix ExhaustMemory on clang. BUG=681218 ========== to ========== Fix ExhaustMemory on clang. Also, bolster the current tests to verify sad tab behavior. BUG=681218 ==========
Patchset #9 (id:160001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #6 (id:140001) has been deleted
Patchset #5 (id:120001) has been deleted
On 2017/01/26 18:25:19, Nico wrote: > On 2017/01/26 18:12:44, Will Harris wrote: > > > https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... > > File base/process/memory_unittest.cc (right): > > > > > https://codereview.chromium.org/2648423006/diff/40001/base/process/memory_uni... > > base/process/memory_unittest.cc:151: } > > On 2017/01/26 17:56:53, Nico wrote: > > > do we really need this test? it duplicates the function under test and it > adds > > a > > > death test, which both seems a bit suboptimal. i'd either revert changes to > > this > > > file, or make it so that the old cold calls the function that's actually > being > > > tested here. > > > > I'm not sure I totally understand what you mean by "duplicates the function > > under test" > > It copies ExhaustMemory() from render_frame_impl.cc to memory_unittest.cc > > >, but alternatively I could move the test to a browser test and > > navigate to the URL instead, would that suffice? > > It'd take 30x longer to run and probably be flaky, but that'd work for me if you > prefer that. > > > Historically we've been okay with death tests for the memory code (see the > rest > > of the file), but I agree it's non-optimal to duplicate the ExhaustMemory code > > in two places. > > I guess in my mind I was scoring the test bit as "test coverage: +10 points, > duplicates code under test -10 points, death test -1 point – oh score for test > is below 0, better ask if we really want this test". So any way of not duping > the function makes the test worthwhile. > > Options: > - put function in render_frame_impl header, put test in new > render_frame_impl_unittest.cc (and ref it from content/test/BUILD.gn) … probably > best option > - put function in base, keep test where it is, make render_frame_impl.cc call > that. con: new one-time-use function in base > - write browser test instead. pro: integration test for what we're shipping. > con: takes much longer to run and is likely more flaky I went with the browser test option, and found a suitable test I could attach it to. PTAL
wfh@chromium.org changed reviewers: + isherman@chromium.org
isherman can you take a look at the addition to chrome/browser/metrics/metrics_service_browsertest.cc ? thanks
lgtm!
Sorry, won't have a chance to look at this today, will try to get to it tomorrow!
Thanks, Will. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/metrics_service_browsertest.cc (right): https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:42: namespace { nit: Please leave a blank space after this line. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:45: void verifyRendererExitCodeIsSignal(const base::HistogramTester& hist_tester, nit: Please start this method with an Uppercase letter https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:45: void verifyRendererExitCodeIsSignal(const base::HistogramTester& hist_tester, nit: Please spell out "histogram" rather than "hist" (applies throughout) https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:50: int32_t exit_code = buckets[0].min; Can you use hist_tester.ExpectUniqueSample() in place of these four lines? Or is it harder to map from the signal to the expected exit code? https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:176: EXPECT_EQ(4, prefs->GetInteger(metrics::prefs::kStabilityPageLoadCount)); nit: Mebbe document where this page load count is coming from?
wfh@chromium.org changed reviewers: + jam@chromium.org
Thanks for review - sorry there's a rebase hidden in there. Also, looks like I need a RS on the change in content/ -> jam@ PTAL. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/metrics_service_browsertest.cc (right): https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:42: namespace { On 2017/02/07 16:50:26, Ilya Sherman wrote: > nit: Please leave a blank space after this line. Done. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:45: void verifyRendererExitCodeIsSignal(const base::HistogramTester& hist_tester, On 2017/02/07 16:50:26, Ilya Sherman wrote: > nit: Please spell out "histogram" rather than "hist" (applies throughout) Done. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:45: void verifyRendererExitCodeIsSignal(const base::HistogramTester& hist_tester, On 2017/02/07 16:50:26, Ilya Sherman wrote: > nit: Please start this method with an Uppercase letter Done. Not sure how that happened... too much go in my life. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:50: int32_t exit_code = buckets[0].min; On 2017/02/07 16:50:26, Ilya Sherman wrote: > Can you use hist_tester.ExpectUniqueSample() in place of these four lines? Or > is it harder to map from the signal to the expected exit code? yup I can't use the existing APIs because I need to pull out the bucket and then perform an operation on it (the WTERMSIG takes the low 4 bits). Whether there is a core dump (0x80 bit) or not depends on system configuration at the time. If I knew that the expected signal was always going to be SIGSEGV (11) or SIGSEGV | WCOREFLAG (139) then I could use the normal functions, but I wanted to keep this function flexible as it's likely once OOM gets added to Linux, then the function will have to be called with a non-SIGSEGV parameter. https://codereview.chromium.org/2648423006/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:176: EXPECT_EQ(4, prefs->GetInteger(metrics::prefs::kStabilityPageLoadCount)); On 2017/02/07 16:50:26, Ilya Sherman wrote: > nit: Mebbe document where this page load count is coming from? Hmm this is copied from above :) tbh I'm not sure where 4 comes from I just copy'n'paste, but I can try and work it out if you want.
LGTM % remaining nits. Thanks! https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/metrics_service_browsertest.cc (right): https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:51: EXPECT_EQ(1UL, buckets.size()); nit: This should be an ASSERT_EQ, since you index into the array below. (Though, TBH, I'm not sure why we bother to distinguish between ASSERT and EXPECT... it's never made much sense to me.) https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:112: base::HistogramTester hist_tester; nit: s/hist/histogram (and below, too).
lgtm
all done, sorry for delay. thanks for reviews. https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/metrics_service_browsertest.cc (right): https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:51: EXPECT_EQ(1UL, buckets.size()); On 2017/02/07 19:40:22, Ilya Sherman wrote: > nit: This should be an ASSERT_EQ, since you index into the array below. > (Though, TBH, I'm not sure why we bother to distinguish between ASSERT and > EXPECT... it's never made much sense to me.) Done. ASSERT halts the test, while EXPECT doesn't - and it makes complete sense to halt the test if the size is not 1, to avoid a crash on line 52. Thanks for noticing. https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:112: base::HistogramTester hist_tester; On 2017/02/07 19:40:22, Ilya Sherman wrote: > nit: s/hist/histogram (and below, too). Done.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, isherman@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2648423006/#ps240001 (title: "final nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(Still LGTM, dear commit bot) https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/metrics_service_browsertest.cc (right): https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_service_browsertest.cc:51: EXPECT_EQ(1UL, buckets.size()); On 2017/02/13 19:24:22, Will Harris wrote: > On 2017/02/07 19:40:22, Ilya Sherman wrote: > > nit: This should be an ASSERT_EQ, since you index into the array below. > > (Though, TBH, I'm not sure why we bother to distinguish between ASSERT and > > EXPECT... it's never made much sense to me.) > > Done. ASSERT halts the test, while EXPECT doesn't - and it makes complete sense > to halt the test if the size is not 1, to avoid a crash on line 52. Thanks for > noticing. Yep -- what I meant is: Is it really worth having assertions that don't halt the test? It's slightly more expressive, but I'm not convinced it's worth the complexity. Anyway, tangent point of discussion is tangent =)
On 2017/02/13 19:27:21, Ilya Sherman wrote: > (Still LGTM, dear commit bot) > > https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... > File chrome/browser/metrics/metrics_service_browsertest.cc (right): > > https://codereview.chromium.org/2648423006/diff/220001/chrome/browser/metrics... > chrome/browser/metrics/metrics_service_browsertest.cc:51: EXPECT_EQ(1UL, > buckets.size()); > On 2017/02/13 19:24:22, Will Harris wrote: > > On 2017/02/07 19:40:22, Ilya Sherman wrote: > > > nit: This should be an ASSERT_EQ, since you index into the array below. > > > (Though, TBH, I'm not sure why we bother to distinguish between ASSERT and > > > EXPECT... it's never made much sense to me.) > > > > Done. ASSERT halts the test, while EXPECT doesn't - and it makes complete > sense > > to halt the test if the size is not 1, to avoid a crash on line 52. Thanks for > > noticing. > > Yep -- what I meant is: Is it really worth having assertions that don't halt the > test? It's slightly more expressive, but I'm not convinced it's worth the > complexity. Anyway, tangent point of discussion is tangent =) well sometimes it's nice to know if e.g. there are ten items being tested in order, and only five fail then which of the five... anyway, tangent tangent...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487013880843120,
"parent_rev": "ccbbfa2f4c2f0c8f5736d05a2f9886700d79e738", "commit_rev":
"3dcb0a73e6f194be62930e7e1924a0b6965f5637"}
Message was sent while issue was closed.
Description was changed from ========== Fix ExhaustMemory on clang. Also, bolster the current tests to verify sad tab behavior. BUG=681218 ========== to ========== Fix ExhaustMemory on clang. Also, bolster the current tests to verify sad tab behavior. BUG=681218 Review-Url: https://codereview.chromium.org/2648423006 Cr-Commit-Position: refs/heads/master@{#450078} Committed: https://chromium.googlesource.com/chromium/src/+/3dcb0a73e6f194be62930e7e1924... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3dcb0a73e6f194be62930e7e1924... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
