Creating a local PartitionAllocatorGeneric instance should be allowed. |
|
Issue descriptionSummary: ======== It's very useful for tests. However, initializing a local PartitionAllocatorGeneric will non-deterministically deadlock unless the instance is memset to 0. Longer explanation: =================== PartitionAllocatorGeneric is trivially constructible. Its member PartitionRootGeneric has a member base::subtle::SpinLock, which has a giant warning: """ // |SpinLock|s MUST be globals. Using them as (e.g.) struct/class members will // result in an uninitialized lock, which is dangerously incorrect. """ As such, it's also only legal to have global PartitionAllocatorGeneric instances. A quick search through the codebase shows this is mostly true: https://cs.chromium.org/search/?q=PartitionAllocatorGeneric&sq=package:chromium&type=cs With the exception of partition_alloc_unittest.cc. That test only works because of a memset: """ void SetUp() override { // TODO( crbug.com/722911 ): These calls to |memset| should perhaps not be // necessary. memset(&allocator, 0, sizeof(allocator)); memset(&generic_allocator, 0, sizeof(generic_allocator)); allocator.init(); generic_allocator.init(); } """ The linked crbug seems to reference an unrelated issue related to OOMs from running parallel tests. Proposed Solutions: =================== 1) IMO, the reasonable solution is to give SpinLock() a constructor that correctly initializes |lock_|. The extra cycles needed at global startup are minimal. However, given that someone went to all the effort to put a giant warning on the class... 2) Another acceptable solution would be to make PartitionAllocGenericInit initialize the lock. I'm going to implement (2), and then the OWNERs of Partition Alloc are welcome to change it to (1) or something else.
,
Sep 19 2017
It turns out that the CL I landed does not perform sufficient initialization to allow creation of local instances of PartitionAllocatorGeneric on all platforms. There's other state such as PartitionRootBase::initialized that also need to be set to 0. Given that all PartitionAlloc instances have a non-trivial init() method that needs to be called before they can be used...it's not clear to me why we're not using standard object constructors. I traced this back in history...it looks like when the lock was first introduced, there was explicit initialization: https://codereview.chromium.org/21666003/diff/16001/Source/wtf/PartitionAlloc.cpp """ @@ -46,6 +46,7 @@ void partitionAllocInit(PartitionRoot* root) { ASSERT(!root->initialized); root->initialized = true; + root->lock = 0; """ I'm going to change all PartitionAlloc-related objects to have constructors and not rely on being instantiated as a global. The performance impact is minimal, given that PartitionAlloc*::init() methods are non-trivial.
,
Sep 19 2017
#2: That plan SGTM. It's been in the back of my mind for a while.
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61538ef94febd23ee6ea84f0851489961661a6db commit 61538ef94febd23ee6ea84f0851489961661a6db Author: Erik Chen <erikchen@chromium.org> Date: Wed Sep 20 21:41:45 2017 Refactor PartitionAllocator to allow local instances. Previously, PartitionAllocator and related classes were trivially constructible, but relied on 0-initialization of internal fields. This meant that only global instances of PartitionAllocator could be used. This makes testing PartitionAllocator difficult. This CL adds constructors to PartitionAllocator classes, and switches globals to LazyInstances, but otherwise should not cause behavior change. The expected performance impact should be unnoticeable, as PartitionAllocator already has non-trivial initialization code. This CL should not affect performance of the allocation path itself. Bug: 765735, 722911 Change-Id: I8cd456772a3d71d316161a67d966e89fb9b81a63 Reviewed-on: https://chromium-review.googlesource.com/673726 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#503257} [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/address_space_randomization.cc [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/page_allocator.cc [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/partition_alloc.cc [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/partition_alloc.h [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/partition_alloc_unittest.cc [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/spin_lock.cc [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/base/allocator/partition_allocator/spin_lock.h [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/third_party/WebKit/Source/platform/wtf/allocator/Partitions.cpp [modify] https://crrev.com/61538ef94febd23ee6ea84f0851489961661a6db/third_party/WebKit/Source/platform/wtf/allocator/Partitions.h
,
Oct 17 2017
Hi Erik, I'm trying to figure out what is happening on this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=768302 I'm not sure I understand why the spinlock needs to be a LazyInstance in this case.
,
Oct 17 2017
Usually in Chrome, we use LazyInstances to allocate objects with non-trivial constructors. The implementation of SpinLock needs to be 0-initialized. I changed this to happen in a constructor, rather than implicitly relying on 0-initialization, which was sometimes, but not always true [e.g. in tests SpinLocks were not being 0-initialized]. |
|
►
Sign in to add a comment |
|
Comment 1 by bugdroid1@chromium.org
, Sep 15 2017