PartitionAlloc: Increase the number of pages per bucket

PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

This CL increase the limit to 2^24 instead of decreasing the limit of
|numSystemPagesPerSlotSpan| to 2^8 (It is statically guaranteed that |numSystemPagesPerSlotSpan|
does not exceed 2^8). As a result, this CL doesn't change sizeof(PartitionBucket).

BUG=87772
Committed: https://crrev.com/63f7a4e7c4255204a90569f7ace2d3aeddcea22e
Cr-Commit-Position: refs/heads/master@{#371745}

[haraken, 2016-01-22 18:38:46]

Description was changed from

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

As far as I understand, the reason we wanted to limite the number to 2^16
is to slim down sizeof(PartitionBucket). However, I don't think that the
sizeof(PartitionBucket)
matters becuase we allocate only one PartitionBucket per bucket.
Even if we increae the number to 2^32, it wouldn't cause any performance or
memory regression (I hope).

BUG=87772
==========

to

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

As far as I understand, the reason we wanted to limite the number to 2^16
is to slim down sizeof(PartitionBucket). However, I don't think that the
sizeof(PartitionBucket)
matters becuase we allocate only one PartitionBucket per bucket.
Even if we increase the number to 2^32, it wouldn't cause any performance or
memory regression (I hope).

BUG=87772
==========

[haraken, 2016-01-22 18:42:07]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org, tyoshino@chromium.org

[haraken, 2016-01-22 18:42:08]

PTAL

In terms of memory, I'm pretty sure that this CL won't regress anything. The
number of PartitionBuckets allocated in PartitionAlloc is limited.

In terms of performance, I don't think this CL will regress anything but I'll
keep an eye on the perf bots.

[haraken, 2016-01-22 23:03:43]

It looks like I need to fix crashes in the bots.

[haraken, 2016-01-25 05:27:25]

Description was changed from

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

As far as I understand, the reason we wanted to limite the number to 2^16
is to slim down sizeof(PartitionBucket). However, I don't think that the
sizeof(PartitionBucket)
matters becuase we allocate only one PartitionBucket per bucket.
Even if we increase the number to 2^32, it wouldn't cause any performance or
memory regression (I hope).

BUG=87772
==========

to

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

This CL increase the limit to 2^24 instead of decreasing the limit of
|numSystemPagesPerSlotSpan| to 2^8 (It is statically guaranteed that
|numSystemPagesPerSlotSpan|
does not exceed 2^8). As a result, this CL doesn't change
sizeof(PartitionBucket).

BUG=87772
==========

[haraken, 2016-01-25 05:32:31]

OK, I invented a better idea that doesn't increase sizeof(PartitionBucket) :)

PTAL at PS2.

[haraken, 2016-01-25 05:32:48]

On 2016/01/25 05:32:31, haraken wrote:
> OK, I invented a better idea that doesn't increase sizeof(PartitionBucket) :)
> 
> PTAL at PS2.

s/PS2/PS3/.

[tyoshino (SeeGerritForStatus), 2016-01-25 05:42:20]

With PS3 applied, Chrome doesn't crash for the test case by wyantb (
http://code.google.com/p/chromium/issues/detail?id=87772#c60 ).

[bashi, 2016-01-26 23:52:38]

LGTM with nits

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82: RELEASE_ASSERT(bestPages <
(1 << 8));
RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.cpp:102: RELEASE_ASSERT(bestPages <
(1 << 8));
ditto.

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.h:262: unsigned
numSystemPagesPerSlotSpan : 8;
uint32_t ?

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.h:263: unsigned numFullPages : 24;
ditto.

[haraken, 2016-01-27 01:19:32]

Thanks for the review!

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82: RELEASE_ASSERT(bestPages <
(1 << 8));
On 2016/01/26 23:52:37, bashi1 wrote:
> RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?

No, what I really want to assert here is that the following static_cast<uint8_t>
doesn't lose information (i.e., bestPages is less than 1<<8). Given the current
configuration of PartitionAlloc, it is statically guaranteed, but it may change
if someone changes some constant value of PartitionAlloc. This assertion is here
to catch it.

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/PartitionAlloc.h (right):

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.h:262: unsigned
numSystemPagesPerSlotSpan : 8;
On 2016/01/26 23:52:37, bashi1 wrote:
> uint32_t ?

The style checker asks us to use unsigned for a bit-field.

[haraken, 2016-01-27 01:19:44]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-01-27 01:22:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1622553004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1622553004/40001

[bashi, 2016-01-27 01:34:07]

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82: RELEASE_ASSERT(bestPages <
(1 << 8));
On 2016/01/27 01:19:32, haraken wrote:
> On 2016/01/26 23:52:37, bashi1 wrote:
> > RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?
> 
> No, what I really want to assert here is that the following
static_cast<uint8_t>
> doesn't lose information (i.e., bestPages is less than 1<<8). Given the
current
> configuration of PartitionAlloc, it is statically guaranteed, but it may
change
> if someone changes some constant value of PartitionAlloc. This assertion is
here
> to catch it.
> 

I think it's not the most important thing to ensure bestPages is less than 1<<8.
What we really want to do here is to ensure that we don't exceed the maximum
number of |PartitionBucket::numSystemPagesPerSlotSpan|, which is logically
determined by the size and relationship of system pages and slot spans.  8 bits
are just enough bits to store that maximum number.

[haraken, 2016-01-27 01:35:50]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2016-01-27 01:38:58]

On 2016/01/27 01:34:07, bashi1 wrote:
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82: RELEASE_ASSERT(bestPages
<
> (1 << 8));
> On 2016/01/27 01:19:32, haraken wrote:
> > On 2016/01/26 23:52:37, bashi1 wrote:
> > > RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?
> > 
> > No, what I really want to assert here is that the following
> static_cast<uint8_t>
> > doesn't lose information (i.e., bestPages is less than 1<<8). Given the
> current
> > configuration of PartitionAlloc, it is statically guaranteed, but it may
> change
> > if someone changes some constant value of PartitionAlloc. This assertion is
> here
> > to catch it.
> > 
> 
> I think it's not the most important thing to ensure bestPages is less than
1<<8.
> What we really want to do here is to ensure that we don't exceed the maximum
> number of |PartitionBucket::numSystemPagesPerSlotSpan|, which is logically
> determined by the size and relationship of system pages and slot spans.  8
bits
> are just enough bits to store that maximum number.

Then:

  RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan);

and:

  static_assert(kMaxSystemPagesPerSlotSpan < (1<<8));

?

(I was rather worrying about the former condition since we're truncating the
bestPages into uint8_t.)

[haraken, 2016-01-27 01:39:11]

On 2016/01/27 01:38:58, haraken wrote:
> On 2016/01/27 01:34:07, bashi1 wrote:
> >
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82:
RELEASE_ASSERT(bestPages
> <
> > (1 << 8));
> > On 2016/01/27 01:19:32, haraken wrote:
> > > On 2016/01/26 23:52:37, bashi1 wrote:
> > > > RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?
> > > 
> > > No, what I really want to assert here is that the following
> > static_cast<uint8_t>
> > > doesn't lose information (i.e., bestPages is less than 1<<8). Given the
> > current
> > > configuration of PartitionAlloc, it is statically guaranteed, but it may
> > change
> > > if someone changes some constant value of PartitionAlloc. This assertion
is
> > here
> > > to catch it.
> > > 
> > 
> > I think it's not the most important thing to ensure bestPages is less than
> 1<<8.
> > What we really want to do here is to ensure that we don't exceed the maximum
> > number of |PartitionBucket::numSystemPagesPerSlotSpan|, which is logically
> > determined by the size and relationship of system pages and slot spans.  8
> bits
> > are just enough bits to store that maximum number.
> 
> Then:
> 
>   RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan);
> 
> and:
> 
>   static_assert(kMaxSystemPagesPerSlotSpan < (1<<8));
> 
> ?
> 
> (I was rather worrying about the former condition since we're truncating the
> bestPages into uint8_t.)

s/former/latter/

[bashi, 2016-01-27 02:03:47]

On 2016/01/27 01:39:11, haraken wrote:
> On 2016/01/27 01:38:58, haraken wrote:
> > On 2016/01/27 01:34:07, bashi1 wrote:
> > >
> >
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82:
> RELEASE_ASSERT(bestPages
> > <
> > > (1 << 8));
> > > On 2016/01/27 01:19:32, haraken wrote:
> > > > On 2016/01/26 23:52:37, bashi1 wrote:
> > > > > RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?
> > > > 
> > > > No, what I really want to assert here is that the following
> > > static_cast<uint8_t>
> > > > doesn't lose information (i.e., bestPages is less than 1<<8). Given the
> > > current
> > > > configuration of PartitionAlloc, it is statically guaranteed, but it may
> > > change
> > > > if someone changes some constant value of PartitionAlloc. This assertion
> is
> > > here
> > > > to catch it.
> > > > 
> > > 
> > > I think it's not the most important thing to ensure bestPages is less than
> > 1<<8.
> > > What we really want to do here is to ensure that we don't exceed the
maximum
> > > number of |PartitionBucket::numSystemPagesPerSlotSpan|, which is logically
> > > determined by the size and relationship of system pages and slot spans.  8
> > bits
> > > are just enough bits to store that maximum number.
> > 
> > Then:
> > 
> >   RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan);
> > 
> > and:
> > 
> >   static_assert(kMaxSystemPagesPerSlotSpan < (1<<8));
> > 
> > ?
> > 
> > (I was rather worrying about the former condition since we're truncating the
> > bestPages into uint8_t.)
> 
> s/former/latter/

Yeah, I'd prefer having both.

[haraken, 2016-01-27 02:08:28]

On 2016/01/27 02:03:47, bashi1 wrote:
> On 2016/01/27 01:39:11, haraken wrote:
> > On 2016/01/27 01:38:58, haraken wrote:
> > > On 2016/01/27 01:34:07, bashi1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/wtf/PartitionAlloc.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1622553004/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/wtf/PartitionAlloc.cpp:82:
> > RELEASE_ASSERT(bestPages
> > > <
> > > > (1 << 8));
> > > > On 2016/01/27 01:19:32, haraken wrote:
> > > > > On 2016/01/26 23:52:37, bashi1 wrote:
> > > > > > RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan) ?
> > > > > 
> > > > > No, what I really want to assert here is that the following
> > > > static_cast<uint8_t>
> > > > > doesn't lose information (i.e., bestPages is less than 1<<8). Given
the
> > > > current
> > > > > configuration of PartitionAlloc, it is statically guaranteed, but it
may
> > > > change
> > > > > if someone changes some constant value of PartitionAlloc. This
assertion
> > is
> > > > here
> > > > > to catch it.
> > > > > 
> > > > 
> > > > I think it's not the most important thing to ensure bestPages is less
than
> > > 1<<8.
> > > > What we really want to do here is to ensure that we don't exceed the
> maximum
> > > > number of |PartitionBucket::numSystemPagesPerSlotSpan|, which is
logically
> > > > determined by the size and relationship of system pages and slot spans. 
8
> > > bits
> > > > are just enough bits to store that maximum number.
> > > 
> > > Then:
> > > 
> > >   RELEASE_ASSERT(bestPages <= kMaxSystemPagesPerSlotSpan);
> > > 
> > > and:
> > > 
> > >   static_assert(kMaxSystemPagesPerSlotSpan < (1<<8));
> > > 
> > > ?
> > > 
> > > (I was rather worrying about the former condition since we're truncating
the
> > > bestPages into uint8_t.)
> > 
> > s/former/latter/
> 
> Yeah, I'd prefer having both.

Done.

[haraken, 2016-01-27 02:08:31]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-01-27 02:08:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1622553004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-01-27 02:08:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1622553004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1622553004/60001

[commit-bot: I haz the power, 2016-01-27 02:21:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 02:21:50]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[haraken, 2016-01-27 02:29:16]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-01-27 02:29:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1622553004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-01-27 02:30:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1622553004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1622553004/80001

[commit-bot: I haz the power, 2016-01-27 03:23:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 03:23:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[haraken, 2016-01-27 04:27:49]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-01-27 04:27:49]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1622553004/#ps100001
(title: " ")

[haraken, 2016-01-27 04:28:00]

Updated the assertions a bit more.

[commit-bot: I haz the power, 2016-01-27 04:28:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1622553004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1622553004/100001

[commit-bot: I haz the power, 2016-01-27 05:55:59]

Description was changed from

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

This CL increase the limit to 2^24 instead of decreasing the limit of
|numSystemPagesPerSlotSpan| to 2^8 (It is statically guaranteed that
|numSystemPagesPerSlotSpan|
does not exceed 2^8). As a result, this CL doesn't change
sizeof(PartitionBucket).

BUG=87772
==========

to

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

This CL increase the limit to 2^24 instead of decreasing the limit of
|numSystemPagesPerSlotSpan| to 2^8 (It is statically guaranteed that
|numSystemPagesPerSlotSpan|
does not exceed 2^8). As a result, this CL doesn't change
sizeof(PartitionBucket).

BUG=87772
==========

[commit-bot: I haz the power, 2016-01-27 05:56:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-27 05:56:49]

Description was changed from

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

This CL increase the limit to 2^24 instead of decreasing the limit of
|numSystemPagesPerSlotSpan| to 2^8 (It is statically guaranteed that
|numSystemPagesPerSlotSpan|
does not exceed 2^8). As a result, this CL doesn't change
sizeof(PartitionBucket).

BUG=87772
==========

to

==========
PartitionAlloc: Increase the number of pages per bucket

Currently the number of pages per bucket is limited to 2^16,
and this has caused a bunch of crash reports at partitionBucketFull().

This CL increase the limit to 2^24 instead of decreasing the limit of
|numSystemPagesPerSlotSpan| to 2^8 (It is statically guaranteed that
|numSystemPagesPerSlotSpan|
does not exceed 2^8). As a result, this CL doesn't change
sizeof(PartitionBucket).

BUG=87772

Committed: https://crrev.com/63f7a4e7c4255204a90569f7ace2d3aeddcea22e
Cr-Commit-Position: refs/heads/master@{#371745}
==========

[commit-bot: I haz the power, 2016-01-27 05:56:49]

Patchset 6 (id:??) landed as
https://crrev.com/63f7a4e7c4255204a90569f7ace2d3aeddcea22e
Cr-Commit-Position: refs/heads/master@{#371745}