Implement growth policy for old space using time and space signals.

Implement growth policy for old space using time and space signals.

Committed: https://code.google.com/p/dart/source/detail?r=8136

[Ivan Posva, 2012-05-29 23:27:37]

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:99: isolate->heap()->EnableGrowthControl();
This means that the growth policy is only active when starting from a snapshot.
Probably not what you intended.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:76: bool is_executable)
?

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:352: int64_t end = OS::GetCurrentTimeMillis();
Why not use the timer object?

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:396: if (heap_growth_ratio_ == 100) {
For debugging we might want to add a always_grow_to_max instead of this hack.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:399: if (grow_heap_ == 0) {
<= 0

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:91: static const uint32_t kHistoryLength = 16;
I have the feeling (no data) that this longish history will lead to being too
slow to react to changes.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:112: bool CanGrowPageSpace();
Maybe this should be dependent on the amount of memory requested?

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:148: PageSpace(Heap* heap,
?

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:229: bool is_growth_controlled_;
How about tracking this in the controller itself?

[cshapiro, 2012-05-30 17:56:24]

PTAL

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:99: isolate->heap()->EnableGrowthControl();
On 2012/05/29 23:27:37, Ivan Posva wrote:
> This means that the growth policy is only active when starting from a
snapshot.
> Probably not what you intended.

Done.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:76: bool is_executable)
More noise.  Done.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:352: int64_t end = OS::GetCurrentTimeMillis();
Okay.  The timer does not directly provide this data so is somewhat more
verbose.  Let me know what you think.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:396: if (heap_growth_ratio_ == 100) {
Or, I could suppress the enabling of the controller.  Do you want this in the
current CL or...?

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:399: if (grow_heap_ == 0) {
On 2012/05/29 23:27:37, Ivan Posva wrote:
> <= 0

Done.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:91: static const uint32_t kHistoryLength = 16;
I have set this back to 4.  I think 4 is too short of a kernel.  I am not sure
what the right value is without more data.  However, a single spike in the data
will contribute too much to the time threshold whereas a longer history has more
smoothing.  Too much history is not good.

A compromise would be to not abort the averaging when the buffer is partially
filled.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:112: bool CanGrowPageSpace();
Done.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:148: PageSpace(Heap* heap,
Noise.  Done.

https://chromiumcodereview.appspot.com/10442073/diff/1/runtime/vm/pages.h#new...
runtime/vm/pages.h:229: bool is_growth_controlled_;
Sure.  Done.

[Ivan Posva, 2012-05-30 20:21:23]

As discussed we also need to deal with the promotion failures, which was a big
missing piece in the existing code.

-Ivan

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc#newcode202
runtime/vm/pages.cc:202: page_space_controller_.CanGrowPageSpace(kPageSize) &&
Number of pages or byte size?

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc#newcode354
runtime/vm/pages.cc:354: int64_t elapsed = timer.TotalElapsedTime() *
kMicrosecondsPerMillisecond;
As discussed this will only work if --verbose_gc is on.

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc#newcode394
runtime/vm/pages.cc:394: bool PageSpaceController::CanGrowPageSpace(intptr_t
num_pages) {
num_pages or byte size?

[cshapiro, 2012-05-30 23:39:54]

PTAL

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc#newcode202
runtime/vm/pages.cc:202: page_space_controller_.CanGrowPageSpace(kPageSize) &&
Byte size.  Done.

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc#newcode354
runtime/vm/pages.cc:354: int64_t elapsed = timer.TotalElapsedTime() *
kMicrosecondsPerMillisecond;
Made unconditional.  Done.

http://codereview.chromium.org/10442073/diff/1006/runtime/vm/pages.cc#newcode394
runtime/vm/pages.cc:394: bool PageSpaceController::CanGrowPageSpace(intptr_t
num_pages) {
On 2012/05/30 20:21:24, Ivan Posva wrote:
> num_pages or byte size?

Done.