New issue
Advanced search Search tips

Issue 695274 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature



Sign in to add a comment

Make PartitionAlloc entirely self-contained

Project Member Reported by palmer@chromium.org, Feb 23 2017

Issue description

base/allocator/partition_allocator depends on some stuff from base/. It'd be best if it were entirely self-contained, so that non-Chromium projects can easily import it. (1st up: PDFium.)
 

Comment 2 by jsc...@chromium.org, Feb 23 2017

Cc: tsepez@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5caa9cbf4796054a6094d9606a7cca4b0013df94

commit 5caa9cbf4796054a6094d9606a7cca4b0013df94
Author: palmer <palmer@chromium.org>
Date: Tue Feb 28 00:34:17 2017

Move spin_lock into partition_allocator.

This helps make partition_alloc more self-contained.

BUG= 695274 

Review-Url: https://codereview.chromium.org/2713903002
Cr-Commit-Position: refs/heads/master@{#453414}

[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/base/BUILD.gn
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/base/allocator/partition_allocator/address_space_randomization.cc
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/base/allocator/partition_allocator/partition_alloc.cc
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/base/allocator/partition_allocator/partition_alloc.h
[rename] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/base/allocator/partition_allocator/spin_lock.cc
[rename] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/base/allocator/partition_allocator/spin_lock.h
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/third_party/WebKit/Source/platform/wtf/DEPS
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/third_party/WebKit/Source/wtf/DEPS
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/third_party/WebKit/Source/wtf/SpinLock.h
[modify] https://crrev.com/5caa9cbf4796054a6094d9606a7cca4b0013df94/third_party/WebKit/Source/wtf/allocator/Partitions.h

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2101d04be0e3da816f47164e8c5120aaea90e27

commit c2101d04be0e3da816f47164e8c5120aaea90e27
Author: palmer <palmer@chromium.org>
Date: Wed Mar 15 17:11:33 2017

Use std::atomic instead of atopmicops.h in PartitionAlloc.

This results in 1 less dependency for PA.

BUG= 695274 

Review-Url: https://codereview.chromium.org/2751853003
Cr-Commit-Position: refs/heads/master@{#457112}

[modify] https://crrev.com/c2101d04be0e3da816f47164e8c5120aaea90e27/base/allocator/partition_allocator/page_allocator.cc

Comment 5 by palmer@chromium.org, Mar 15 2017

Status: Fixed (was: Started)
I investigated yesterday, and I think this is about as independent as it can get. The remaining dependencies are single header files from base/ which are as easy to copy as to replicate. Calling this Fixed.

Comment 6 by jsc...@chromium.org, Mar 15 2017

Onward to the PDFium!

Comment 7 by palmer@chromium.org, Mar 16 2017

As you can see from https://pdfium-review.googlesource.com/c/3066/, even the 'minimal' base dependencies got a little hairy. (IMMEDIATE_CRASH, for example, suddenly got way more complicated.) I don't see an immediate way around that; I'd be surprised if the base/ OWNERS wanted to commit to a guarantee of minimal intradependence.

We'll see how painful it is going forward; maybe it won't be too bad. But if it gets bad, we might want to do some more work to make PA less base-dependent.
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/136d94d61e825232fccd78145cd94f86f5c20aae

commit 136d94d61e825232fccd78145cd94f86f5c20aae
Author: palmer <palmer@chromium.org>
Date: Tue Apr 11 21:28:55 2017

Don't depend on base::win in PartitionAlloc.

It seemed like a good idea, but we want to be able to use PA in other projects
(like PDFium) without pulling in even more base dependencies. Instead, use the
plain Windows API for getting the version.

BUG= 672219 , 695274 
TBR=haraken

Review-Url: https://codereview.chromium.org/2811903003
Cr-Commit-Position: refs/heads/master@{#463776}

[modify] https://crrev.com/136d94d61e825232fccd78145cd94f86f5c20aae/base/allocator/partition_allocator/address_space_randomization.cc

Sign in to add a comment