Constrain the zone segment pool size

Constrain the zone segment pool size

Added a size constraint to the configuration to limit the segment pool.
This will likely fix the memory alerts from small android devices.

BUG=chromium:655129
Committed: https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6
Cr-Commit-Position: refs/heads/master@{#40476}

[heimbuef, 2016-10-18 11:41:20]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 11:41:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 11:47:20]

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

[commit-bot: I haz the power, 2016-10-18 11:47:20]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[heimbuef, 2016-10-18 11:55:46]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 11:55:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[heimbuef, 2016-10-18 11:58:14]

heimbuef@google.com changed reviewers:
+ mlippautz@chromium.org

[heimbuef, 2016-10-18 11:58:14]

PTAL

[heimbuef, 2016-10-18 11:58:51]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-18 12:02:00]

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

[commit-bot: I haz the power, 2016-10-18 12:02:01]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16307)

[heimbuef, 2016-10-18 12:59:58]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 13:00:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 13:14:17]

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

[commit-bot: I haz the power, 2016-10-18 13:14:18]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16318)

[heimbuef, 2016-10-18 14:39:10]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 14:39:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Lippautz, 2016-10-18 14:50:21]

mlippautz@chromium.org changed reviewers:
+ jochen@chromium.org, verwaest@chromium.org

[Michael Lippautz, 2016-10-18 14:50:22]

Fine with me if you use existing classes for ResourceConstraints. 

+jochen,toon since I have not reviewed any of the general zone optimizations

https://codereview.chromium.org/2424393002/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2424393002/diff/60001/include/v8.h#newcode5704
include/v8.h:5704: size_t max_pool_size_;
Add initialization to
  ResourceConstraints::ResourceConstraints()
in api.cc

https://codereview.chromium.org/2424393002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2424393002/diff/60001/src/api.cc#newcode748
src/api.cc:748: if (physical_memory <= 512ul * i::MB) {
Use the categories already defined above, starting at line 722.

[heimbuef, 2016-10-18 15:05:25]

On 2016/10/18 at 14:50:22, mlippautz wrote:
> Fine with me if you use existing classes for ResourceConstraints. 
> 
> +jochen,toon since I have not reviewed any of the general zone optimizations
> 
> https://codereview.chromium.org/2424393002/diff/60001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/2424393002/diff/60001/include/v8.h#newcode5704
> include/v8.h:5704: size_t max_pool_size_;
> Add initialization to
>   ResourceConstraints::ResourceConstraints()
> in api.cc
> 
> https://codereview.chromium.org/2424393002/diff/60001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/2424393002/diff/60001/src/api.cc#newcode748
> src/api.cc:748: if (physical_memory <= 512ul * i::MB) {
> Use the categories already defined above, starting at line 722.

Done.

[heimbuef, 2016-10-18 15:43:40]

Description was changed from

==========
Constrain the zone segment pool size

Added a size constraint to the configuration to limit the segment pool.
This will likely fix the memory alerts from small android devices.
==========

to

==========
Constrain the zone segment pool size

Added a size constraint to the configuration to limit the segment pool.
This will likely fix the memory alerts from small android devices.

BUG=chromium:655129
==========

[Michael Lippautz, 2016-10-19 08:56:06]

lgtm once comment is addressed.

Please also get one of the others to look.

https://codereview.chromium.org/2424393002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2424393002/diff/80001/src/api.cc#newcode748
src/api.cc:748: if (physical_memory <= low_limit) {
And by "use categories already defined above", I actually meant: merge the if
blocks :)

[heimbuef, 2016-10-19 09:12:54]

On 2016/10/19 at 08:56:06, mlippautz wrote:
> lgtm once comment is addressed.
> 
> Please also get one of the others to look.
> 
> https://codereview.chromium.org/2424393002/diff/80001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/2424393002/diff/80001/src/api.cc#newcode748
> src/api.cc:748: if (physical_memory <= low_limit) {
> And by "use categories already defined above", I actually meant: merge the if
blocks :)

Done.

[heimbuef, 2016-10-19 09:15:09]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 09:15:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 10:29:01]

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

[commit-bot: I haz the power, 2016-10-19 10:29:02]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-19 14:18:32]

it's unclear to me how ConfigureSegmentPool is supposed to keep the pool size
below a certain threshold without at the same time making the pool unnecessarily
small, i.e., it seems that we reserve space for larger segments that cannot be
used for smaller ones, even if we only have small ones?

https://codereview.chromium.org/2424393002/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2424393002/diff/120001/include/v8.h#newcode5689
include/v8.h:5689: 
unrelated?

https://codereview.chromium.org/2424393002/diff/120001/include/v8.h#newcode5695
include/v8.h:5695: size_t max_pool_size() const { return max_pool_size_; }
can you pick a less generic name please? We have many pools

https://codereview.chromium.org/2424393002/diff/120001/include/v8.h#newcode5696
include/v8.h:5696: void set_max_pool_size(const size_t value) { max_pool_size_ =
value; }
please use pool_size_in_bytes or whatever the right unit is instead of just
value

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
File src/zone/accounting-allocator.cc (right):

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
src/zone/accounting-allocator.cc:17: static const uint8_t kDefaultBucketMaxSize
= 5;
also in this file, please don't unnecessarily constrain the bitwidth

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
src/zone/accounting-allocator.cc:48: for (uint8_t power = 0; power <
kNumberBuckets; power++) {
++power (same below)

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
src/zone/accounting-allocator.cc:62: CHECK_LE(total_size, max_pool_size);
this really should be a unit test

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
File src/zone/accounting-allocator.h (right):

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
src/zone/accounting-allocator.h:53: static const uint8_t kNumberBuckets =
size_t please

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
src/zone/accounting-allocator.h:70: uint8_t
unused_segments_sizes[kNumberBuckets];
same here

https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
src/zone/accounting-allocator.h:70: uint8_t
unused_segments_sizes[kNumberBuckets];
same here

also, members should end in _

[heimbuef, 2016-10-20 08:47:30]

On 2016/10/19 at 14:18:32, jochen wrote:
> it's unclear to me how ConfigureSegmentPool is supposed to keep the pool size
below a certain threshold without at the same time making the pool unnecessarily
small, i.e., it seems that we reserve space for larger segments that cannot be
used for smaller ones, even if we only have small ones?
> 
> https://codereview.chromium.org/2424393002/diff/120001/include/v8.h
> File include/v8.h (right):
> 
>
https://codereview.chromium.org/2424393002/diff/120001/include/v8.h#newcode5689
> include/v8.h:5689: 
> unrelated?
> 
>
https://codereview.chromium.org/2424393002/diff/120001/include/v8.h#newcode5695
> include/v8.h:5695: size_t max_pool_size() const { return max_pool_size_; }
> can you pick a less generic name please? We have many pools
> 
>
https://codereview.chromium.org/2424393002/diff/120001/include/v8.h#newcode5696
> include/v8.h:5696: void set_max_pool_size(const size_t value) { max_pool_size_
= value; }
> please use pool_size_in_bytes or whatever the right unit is instead of just
value
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> File src/zone/accounting-allocator.cc (right):
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> src/zone/accounting-allocator.cc:17: static const uint8_t
kDefaultBucketMaxSize = 5;
> also in this file, please don't unnecessarily constrain the bitwidth
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> src/zone/accounting-allocator.cc:48: for (uint8_t power = 0; power <
kNumberBuckets; power++) {
> ++power (same below)
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> src/zone/accounting-allocator.cc:62: CHECK_LE(total_size, max_pool_size);
> this really should be a unit test
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> File src/zone/accounting-allocator.h (right):
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> src/zone/accounting-allocator.h:53: static const uint8_t kNumberBuckets =
> size_t please
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> src/zone/accounting-allocator.h:70: uint8_t
unused_segments_sizes[kNumberBuckets];
> same here
> 
>
https://codereview.chromium.org/2424393002/diff/120001/src/zone/accounting-al...
> src/zone/accounting-allocator.h:70: uint8_t
unused_segments_sizes[kNumberBuckets];
> same here
> 
> also, members should end in _

All done.

[heimbuef, 2016-10-20 09:42:14]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 09:42:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 10:14:52]

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

[commit-bot: I haz the power, 2016-10-20 10:14:53]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-20 10:29:34]

lgtm with comments

https://codereview.chromium.org/2424393002/diff/140001/src/zone/accounting-al...
File src/zone/accounting-allocator.h (right):

https://codereview.chromium.org/2424393002/diff/140001/src/zone/accounting-al...
src/zone/accounting-allocator.h:90: friend class SegmentPoolTests;
add #include "testing/gtest/include/gtest/gtest_prod.h"

and the use FRIEND_TEST(Zone, SegmentPoolConstraints);

the friend declaration should also come right after private:

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/unittes...
File test/unittests/unittests.gyp (right):

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/unittes...
test/unittests/unittests.gyp:120: 'zone/segmentpool-tests.cc',
can you name the file -unittest.cc like the others please?

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/zone/se...
File test/unittests/zone/segmentpool-tests.cc (right):

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/zone/se...
test/unittests/zone/segmentpool-tests.cc:4: #include
"src/zone/accounting-allocator.h"
empty line above this one

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/zone/se...
test/unittests/zone/segmentpool-tests.cc:6: namespace v8 {
empty line above this one

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/zone/se...
test/unittests/zone/segmentpool-tests.cc:30: CHECK_LE(total_size, size);
use EXPECT_LE - there's no reason to crash

https://codereview.chromium.org/2424393002/diff/140001/test/unittests/zone/se...
test/unittests/zone/segmentpool-tests.cc:36:
SegmentPoolTests::test_pool_size_constraints();
just inline the entire static method here

[heimbuef, 2016-10-20 14:16:34]

The CQ bit was checked by heimbuef@google.com

[heimbuef, 2016-10-20 14:16:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2424393002/#ps160001
(title: "Reaction to comments")

[commit-bot: I haz the power, 2016-10-20 14:16:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 14:48:19]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-11-17 22:08:27]

Description was changed from

==========
Constrain the zone segment pool size

Added a size constraint to the configuration to limit the segment pool.
This will likely fix the memory alerts from small android devices.

BUG=chromium:655129
==========

to

==========
Constrain the zone segment pool size

Added a size constraint to the configuration to limit the segment pool.
This will likely fix the memory alerts from small android devices.

BUG=chromium:655129

Committed: https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6
Cr-Commit-Position: refs/heads/master@{#40476}
==========

[commit-bot: I haz the power, 2016-11-17 22:08:28]

Patchset 8 (id:??) landed as
https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6
Cr-Commit-Position: refs/heads/master@{#40476}