Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618853002:
Don't run memory-intensive tests on low-memory devices. (Closed)
|
Created:
3 years, 11 months ago by palmer Modified:
3 years, 11 months ago CC:
Mads Ager (chromium), chromium-reviews, Dai Mikurube (NOT FULLTIME), kouhei+heap_chromium.org, oilpan-reviews, vmpstr+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't run memory-intensive tests on low-memory devices.
It makes the tests flaky. Add a TODO to disable OOM killing for these tests,
where possible.
BUG=675255
Review-Url: https://codereview.chromium.org/2618853002
Cr-Commit-Position: refs/heads/master@{#441802}
Committed: https://chromium.googlesource.com/chromium/src/+/e46baf31c8607e58385f55a79b31e941d29e204f
Patch Set 1 #Patch Set 2 : Make GenericAllocGetSize a no-op, too. #
Total comments: 4
Patch Set 3 : Respond to comments. #
Total comments: 4
Messages
Total messages: 22 (13 generated)
palmer@chromium.org changed reviewers: + haraken@chromium.org, primiano@chromium.org
This should be a quick review. Thanks!
The CQ bit was checked by palmer@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 palmer@chromium.org
The CQ bit was checked by palmer@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...
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2618853002/diff/20001/base/allocator/partitio... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2618853002/diff/20001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:614: // TODO: Where necessary and possible, disable the platform's OOM-killing TODO, unlike the old FIXME, always comes with a name or bug link. ie the syntax is "TODO(crbug.com/678782): " https://codereview.chromium.org/2618853002/diff/20001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:718: // behavior. OOM-killing makes this test flaky on low-memory devices. If it's flaky shouldn't it be disabled, or is the comment wrong here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with dana's comments addressed.
https://codereview.chromium.org/2618853002/diff/20001/base/allocator/partitio... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2618853002/diff/20001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:614: // TODO: Where necessary and possible, disable the platform's OOM-killing > TODO, unlike the old FIXME, always comes with a name or bug link. ie the syntax is "TODO(crbug.com/678782): " Done. https://codereview.chromium.org/2618853002/diff/20001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:718: // behavior. OOM-killing makes this test flaky on low-memory devices. > If it's flaky shouldn't it be disabled, or is the comment wrong here. Oops, forgot to put the actual code in. Sigh.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2618853002/#ps40001 (title: "Respond to 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": 40001, "attempt_start_ts": 1483662784555030,
"parent_rev": "b28b2b66578c862d563727cc3f30cf84aae942f0", "commit_rev":
"e46baf31c8607e58385f55a79b31e941d29e204f"}
Message was sent while issue was closed.
Description was changed from ========== Don't run memory-intensive tests on low-memory devices. It makes the tests flaky. Add a TODO to disable OOM killing for these tests, where possible. BUG=675255 ========== to ========== Don't run memory-intensive tests on low-memory devices. It makes the tests flaky. Add a TODO to disable OOM killing for these tests, where possible. BUG=675255 Review-Url: https://codereview.chromium.org/2618853002 Cr-Commit-Position: refs/heads/master@{#441802} Committed: https://chromium.googlesource.com/chromium/src/+/e46baf31c8607e58385f55a79b31... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e46baf31c8607e58385f55a79b31...
Message was sent while issue was closed.
https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:614: // TODO(crbug.com/678782): Where necessary and possible, disable the I don't think this is needed. If you look in the body of this test, I did already add a narrower exclusion below only on the 512M part. According to crbug.com/675255#c9: "In all of these, PartitionAllocTest.GenericAllocGetSize and PartitionAllocTest.GenericAllocSizes are passing now (whereas they were failing 100% before), so these I would say are fixed (Woo! Thanks!)." So I'd partially revert this hunk to avoid losing unnecessary coverage. https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:720: if (!IsLargeMemoryDevice()) ditto, I added a narrower exclusion below on the part that triggers OOM https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:1285: return; Maybe add a LOG(WARNING) << "Skipping this test on this device because of crbug.com/678782) so by looking at the buildbot logs one doesn't get the false feeling that this is just working
Message was sent while issue was closed.
On 2017/01/06 10:58:37, Primiano Tucci (back but slow) wrote: > https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... > File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): > > https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... > base/allocator/partition_allocator/partition_alloc_unittest.cc:614: // > TODO(crbug.com/678782): Where necessary and possible, disable the > I don't think this is needed. If you look in the body of this test, I did > already add a narrower exclusion below only on the 512M part. According to > crbug.com/675255#c9: > > "In all of these, PartitionAllocTest.GenericAllocGetSize and > PartitionAllocTest.GenericAllocSizes are passing now (whereas they were failing > 100% before), so these I would say are fixed (Woo! Thanks!)." > > So I'd partially revert this hunk to avoid losing unnecessary coverage. > > https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... > base/allocator/partition_allocator/partition_alloc_unittest.cc:720: if > (!IsLargeMemoryDevice()) > ditto, I added a narrower exclusion below on the part that triggers OOM > > https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... > base/allocator/partition_allocator/partition_alloc_unittest.cc:1285: return; > Maybe add a LOG(WARNING) << "Skipping this test on this device because of > crbug.com/678782) so by looking at the buildbot logs one doesn't get the false > feeling that this is just working Ok doing that myself in crrev.com/2616063002 I spent the last 2.5 days catching up with emails, I need to write some code and this is my opportunity XD
Message was sent while issue was closed.
LGTM anyhow https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2618853002/diff/40001/base/allocator/partitio... base/allocator/partition_allocator/partition_alloc_unittest.cc:614: // TODO(crbug.com/678782): Where necessary and possible, disable the On 2017/01/06 10:58:36, Primiano Tucci (back but slow) wrote: > I don't think this is needed. If you look in the body of this test, I did > already add a narrower exclusion below only on the 512M part. According to > crbug.com/675255#c9: > > "In all of these, PartitionAllocTest.GenericAllocGetSize and > PartitionAllocTest.GenericAllocSizes are passing now (whereas they were failing > 100% before), so these I would say are fixed (Woo! Thanks!)." > > So I'd partially revert this hunk to avoid losing unnecessary coverage. Maybe we can move the TODO down there then |
