Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790173004:
tcmalloc_unittest: Don't run Realloc test on low memory devices (Closed)
Descriptiontcmalloc_unittest: Don't run Realloc test on low memory devices
BUG=b/36238217
TEST=Test not run on low memory device.
Review-Url: https://codereview.chromium.org/2790173004
Cr-Commit-Position: refs/heads/master@{#461805}
Committed: https://chromium.googlesource.com/chromium/src/+/cfd7ac54e35e10f6d52546e18b97e39477eb8013
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #Messages
Total messages: 17 (11 generated)
Description was changed from ========== tcmalloc_unittest: Don't run Realloc test on low memory devices BUG=b/36238217 TEST=Test not run on low memory device. ========== to ========== tcmalloc_unittest: Don't run Realloc test on low memory devices BUG=b/36238217 TEST=Test not run on low memory device. ==========
bcf@chromium.org changed reviewers: + primiano@chromium.org, wfh@chromium.org
This test recently started failing on Chromecast Audio due to OOM. Please let me know what you think. It seems like there's precedence disabling tests like this: https://codereview.chromium.org/2618853002
bcf@chromium.org changed reviewers: + mbjorge@chromium.org
LGTM with comments addressed https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... File base/allocator/tcmalloc_unittest.cc (right): https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... base/allocator/tcmalloc_unittest.cc:91: // 256MiB this is IMHO redundant given the code below, and just prone to end up out of sync :) https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... base/allocator/tcmalloc_unittest.cc:149: if (IsLowMemoryDevice()) to be honest, you could have moved the early return to line 165. the top half is quite small (NextSize goes up to 130k). the problem should be only the 1<<14 below, which * 8192 ends up using 130 MB. In any case, can you please add a comment saying: // The logic below tries to allocate kNumEntries * 9000 ~= 130 MB of memory. This would cause the test to crash on low memory devices with no VM overcommit (e.g., chromecast). https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... base/allocator/tcmalloc_unittest.cc:165: to be honest, you could have moved the early return here. the top half is quite small (NextSize goes up to 130k). the problem should be only the 1<<14 below, which * 8192 ends up using 130 MB.
lgtm
https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... File base/allocator/tcmalloc_unittest.cc (right): https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... base/allocator/tcmalloc_unittest.cc:91: // 256MiB On 2017/04/04 14:10:51, Primiano Tucci wrote: > this is IMHO redundant given the code below, and just prone to end up out of > sync :) Done. https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... base/allocator/tcmalloc_unittest.cc:149: if (IsLowMemoryDevice()) On 2017/04/04 14:10:51, Primiano Tucci wrote: > to be honest, you could have moved the early return to line 165. the top half is > quite small (NextSize goes up to 130k). the problem should be only the 1<<14 > below, which * 8192 ends up using 130 MB. > > In any case, can you please add a comment saying: > > // The logic below tries to allocate kNumEntries * 9000 ~= 130 MB of memory. > This would cause the test to crash on low memory devices with no VM overcommit > (e.g., chromecast). Done. https://codereview.chromium.org/2790173004/diff/1/base/allocator/tcmalloc_uni... base/allocator/tcmalloc_unittest.cc:165: On 2017/04/04 14:10:51, Primiano Tucci wrote: > to be honest, you could have moved the early return here. the top half is quite > small (NextSize goes up to 130k). the problem should be only the 1<<14 below, > which * 8192 ends up using 130 MB. Done.
The CQ bit was checked by bcf@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: This issue passed the CQ dry run.
The CQ bit was checked by bcf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, mbjorge@chromium.org Link to the patchset: https://codereview.chromium.org/2790173004/#ps20001 (title: "Address 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": 20001, "attempt_start_ts": 1491333338168910,
"parent_rev": "95621958b771f10bac0a7690cdbc6f610573e8a1", "commit_rev":
"cfd7ac54e35e10f6d52546e18b97e39477eb8013"}
Message was sent while issue was closed.
Description was changed from ========== tcmalloc_unittest: Don't run Realloc test on low memory devices BUG=b/36238217 TEST=Test not run on low memory device. ========== to ========== tcmalloc_unittest: Don't run Realloc test on low memory devices BUG=b/36238217 TEST=Test not run on low memory device. Review-Url: https://codereview.chromium.org/2790173004 Cr-Commit-Position: refs/heads/master@{#461805} Committed: https://chromium.googlesource.com/chromium/src/+/cfd7ac54e35e10f6d52546e18b97... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/cfd7ac54e35e10f6d52546e18b97... |
