Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689103002:
Fix partition_alloc unit tests. (Closed)
|
Created:
3 years, 10 months ago by sof Modified:
3 years, 10 months ago CC:
chromium-reviews, Dai Mikurube (NOT FULLTIME), Mads Ager (chromium), vmpstr+watch_chromium.org, wfh+watch_chromium.org, oilpan-reviews, kouhei+heap_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix partition_alloc unit tests.
R=haraken
BUG=691197, 684513
Review-Url: https://codereview.chromium.org/2689103002
Cr-Commit-Position: refs/heads/master@{#449892}
Committed: https://chromium.googlesource.com/chromium/src/+/57cc1d8512be5a3719e1f5acbe67a3556ee9c2a7
Patch Set 1 #
Total comments: 9
Messages
Total messages: 23 (11 generated)
The CQ bit was checked by sigbjornf@opera.com 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...
Description was changed from ========== Fix partition_alloc unit tests. R= BUG=691197 ========== to ========== Fix partition_alloc unit tests. R= BUG=691197, 684513 ==========
sigbjornf@opera.com changed reviewers: + haraken@chromium.org, palmer@chromium.org
please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
Description was changed from ========== Fix partition_alloc unit tests. R= BUG=691197, 684513 ========== to ========== Fix partition_alloc unit tests. R=haraken BUG=691197, 684513 ==========
The CQ bit was checked by sigbjornf@opera.com
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": 1, "attempt_start_ts": 1486908476198690, "parent_rev":
"584e0be921ffc96b1b8b400dece9ed18a6af1931", "commit_rev":
"57cc1d8512be5a3719e1f5acbe67a3556ee9c2a7"}
Message was sent while issue was closed.
Description was changed from ========== Fix partition_alloc unit tests. R=haraken BUG=691197, 684513 ========== to ========== Fix partition_alloc unit tests. R=haraken BUG=691197, 684513 Review-Url: https://codereview.chromium.org/2689103002 Cr-Commit-Position: refs/heads/master@{#449892} Committed: https://chromium.googlesource.com/chromium/src/+/57cc1d8512be5a3719e1f5acbe67... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/57cc1d8512be5a3719e1f5acbe67...
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:62: memset(&generic_allocator, 0, sizeof(generic_allocator)); This looks highly suspect; why trash the instance's fields, rather than creating a new instance per-test? https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:1302: // Test "return null" for larger, direct-mapped allocations first. As a Tests normally get sharded across all cores, so their ordering is only approximately followed - is there something causing these tests to be treated specially? https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:1304: // test is performd first for these "return null" tests in order to leave typo: performed https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:1307: // Disable this test on Android because, due to its allocation-heavy behavior, Looks like it is also disabled on Mac - same reason? https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:1319: // Test "return null" with a 512 kB block size. nit: This comment describes what the test *does* not _why_ it does it, i.e. what it intends to test. The comment should be something like "verify return null for a non-direct-mapped allocation size." or similar.
Message was sent while issue was closed.
It would be helpful in future to have the CL description briefly describe the nature of the changes in the patch. Thanks!
Message was sent while issue was closed.
https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:1307: // Disable this test on Android because, due to its allocation-heavy behavior, On 2017/02/12 18:51:28, Wez wrote: > Looks like it is also disabled on Mac - same reason? I believe so; this was just reordering pre-existing comments (as the 2 tests had to be for reasons given above.)
Message was sent while issue was closed.
https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:62: memset(&generic_allocator, 0, sizeof(generic_allocator)); On 2017/02/12 18:51:28, Wez wrote: > This looks highly suspect; why trash the instance's fields, rather than creating > a new instance per-test? The tests use a set of global allocators, which each test initialize by calling TestSetup() before referring to them and tidying up. PA doesn't provide a way to uninitialize their PartitionRoots (or the "allocator" toplevel container structs themselves), so resetting them by way memset() is the effective means to bring that about. Could you rework these unit tests to define their own testing::Test which packaged up these allocators better per-test? Absolutely. Is it needed to address this instability? Don't think so.
Message was sent while issue was closed.
Drive-by nit: Drive-by: In future CLs, could you beef up the commit message with a short explanation of why the change makes sense. I appreciate the bug links which are great, but for code -- especially code in base -- we should ensure why's are recorded in the commit log as well as the code itself before LGTMing and landing. Otherwise code archeology gets really hard. (I just got bit by this in the tracing code today where no one explained why they were forking mutiple ids.) Thanks! On 2017/02/12 20:07:04, sof wrote: > https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... > File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): > > https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... > base/allocator/partition_allocator/partition_alloc_unittest.cc:62: > memset(&generic_allocator, 0, sizeof(generic_allocator)); > On 2017/02/12 18:51:28, Wez wrote: > > This looks highly suspect; why trash the instance's fields, rather than > creating > > a new instance per-test? > > The tests use a set of global allocators, which each test initialize by calling > TestSetup() before referring to them and tidying up. PA doesn't provide a way to > uninitialize their PartitionRoots (or the "allocator" toplevel container structs > themselves), so resetting them by way memset() is the effective means to bring > that about. > > Could you rework these unit tests to define their own testing::Test which > packaged up these allocators better per-test? Absolutely. Is it needed to > address this instability? Don't think so.
Message was sent while issue was closed.
On 2017/02/13 21:38:18, awong wrote: > Drive-by nit: > > Drive-by: In future CLs, could you beef up the commit message with a short > explanation of why the change makes sense. I appreciate the bug links which are > great, but for code -- especially code in base -- we should ensure why's are > recorded in the commit log as well as the code itself before LGTMing and > landing. Otherwise code archeology gets really hard. (I just got bit by this > in the tracing code today where no one explained why they were forking mutiple > ids.) > > Thanks! > Certainly, will make sure I provide more expansive commit messages for base/ (esp); thanks for the gentle nudge.
Message was sent while issue was closed.
https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:62: memset(&generic_allocator, 0, sizeof(generic_allocator)); On 2017/02/12 20:07:04, sof wrote: > On 2017/02/12 18:51:28, Wez wrote: > > This looks highly suspect; why trash the instance's fields, rather than > creating > > a new instance per-test? > > The tests use a set of global allocators, which each test initialize by calling > TestSetup() before referring to them and tidying up. PA doesn't provide a way to > uninitialize their PartitionRoots (or the "allocator" toplevel container structs > themselves), so resetting them by way memset() is the effective means to bring > that about. Presumably we're assuming that the tests themselves don't leak, so we should at least be able to assert that the allocators have no active allocations at test teardown. > Could you rework these unit tests to define their own testing::Test which > packaged up these allocators better per-test? Absolutely. Is it needed to > address this instability? Don't think so. I disagree; we're basically memset()ing over the top of the two structures on the assumption that they only contain POD structures, without clarifying why that is assumed to be safe, or why "traces" from prior tests would break things (though yes, the tests should be hermetic no matter what).
Message was sent while issue was closed.
https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... File base/allocator/partition_allocator/partition_alloc_unittest.cc (right): https://codereview.chromium.org/2689103002/diff/1/base/allocator/partition_al... base/allocator/partition_allocator/partition_alloc_unittest.cc:62: memset(&generic_allocator, 0, sizeof(generic_allocator)); On 2017/02/14 00:24:11, Wez wrote: > On 2017/02/12 20:07:04, sof wrote: > > On 2017/02/12 18:51:28, Wez wrote: > > > This looks highly suspect; why trash the instance's fields, rather than > > creating > > > a new instance per-test? > > > > The tests use a set of global allocators, which each test initialize by > calling > > TestSetup() before referring to them and tidying up. PA doesn't provide a way > to > > uninitialize their PartitionRoots (or the "allocator" toplevel container > structs > > themselves), so resetting them by way memset() is the effective means to bring > > that about. > > Presumably we're assuming that the tests themselves don't leak, so we should at > least be able to assert that the allocators have no active allocations at test > teardown. > > > Could you rework these unit tests to define their own testing::Test which > > packaged up these allocators better per-test? Absolutely. Is it needed to > > address this instability? Don't think so. > > I disagree; we're basically memset()ing over the top of the two structures on > the assumption that they only contain POD structures, without clarifying why > that is assumed to be safe, or why "traces" from prior tests would break things > (though yes, the tests should be hermetic no matter what). OK, looking through the PartitionRoot init code, it seems that nothing is assuming that the data structure was zero'd, so the fact that things fail if they're not seems like a legitimate bug. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
