Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1108133003:
Shrink new space and uncommit from space in idle notification during long idle times. (Closed)
|
Created:
5 years, 7 months ago by Hannes Payer (out of office) Modified:
5 years, 7 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionShrink new space and uncommit from space in idle notification during long idle times.
BUG=chromium:481811
LOG=n
Committed: https://crrev.com/309c082a7380245866576047c30b57794b8690ac
Cr-Commit-Position: refs/heads/master@{#28107}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 22 (10 generated)
hpayer@chromium.org changed reviewers: + eisinger@chromium.org, ulan@chromium.org
lgtm with comment: https://codereview.chromium.org/1108133003/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1108133003/diff/1/src/heap/heap.cc#newcode4634 src/heap/heap.cc:4634: if (long_idle_time) isolate_->compilation_cache()->Clear(); Seems too aggressive. This should happen only if mutator is idle for a long time.
hpayer@chromium.org changed reviewers: + jochen@chromium.org - eisinger@chromium.org
https://codereview.chromium.org/1108133003/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1108133003/diff/1/src/heap/heap.cc#newcode4634 src/heap/heap.cc:4634: if (long_idle_time) isolate_->compilation_cache()->Clear(); On 2015/04/28 10:26:47, ulan wrote: > Seems too aggressive. This should happen only if mutator is idle for a long > time. Done, guarded again with the gc count. But warning: we don't trigger this path often. This should be fixed with the new idle memory phase.
The CQ bit was checked by hpayer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1108133003/#ps20001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108133003/20001
The CQ bit was unchecked by hpayer@chromium.org
The CQ bit was checked by hpayer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108133003/20001
erik.corry@gmail.com changed reviewers: + erik.corry@gmail.com
lgtm
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4507: void Heap::ReduceNewSpaceSize(bool long_idle_time) { nit - would be clearer as is_long_idle_notification https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4516: bool long_idle_time, double idle_time_in_ms, size_t size_of_objects, ditto https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4562: bool long_idle_time = static_cast<size_t>(idle_time_in_ms) > nit - would be clearer as is_long_idle_notification https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4652: ReduceNewSpaceSize(idle_time_in_ms); You are passing idle_time_in_ms to this ReduceNewSpaceSize(bool long_idle_time) - are you sure this is what you want to do? Do you want ReduceNewSpaceSize(idle_time_in_ms > GCIdleTimeHandler::kMaxFrameRenderingIdleTime)
https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4507: void Heap::ReduceNewSpaceSize(bool long_idle_time) { On 2015/04/28 11:19:09, rmcilroy wrote: > nit - would be clearer as is_long_idle_notification Done. https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4516: bool long_idle_time, double idle_time_in_ms, size_t size_of_objects, On 2015/04/28 11:19:08, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4562: bool long_idle_time = static_cast<size_t>(idle_time_in_ms) > On 2015/04/28 11:19:08, rmcilroy wrote: > nit - would be clearer as is_long_idle_notification Done. https://codereview.chromium.org/1108133003/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:4652: ReduceNewSpaceSize(idle_time_in_ms); On 2015/04/28 11:19:08, rmcilroy wrote: > You are passing idle_time_in_ms to this ReduceNewSpaceSize(bool long_idle_time) > - are you sure this is what you want to do? Do you want > ReduceNewSpaceSize(idle_time_in_ms > > GCIdleTimeHandler::kMaxFrameRenderingIdleTime) long_idle_time should be there. thanks, done
lgtm, thanks
The CQ bit was checked by hpayer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org, erik.corry@gmail.com Link to the patchset: https://codereview.chromium.org/1108133003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108133003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/309c082a7380245866576047c30b57794b8690ac Cr-Commit-Position: refs/heads/master@{#28107} |
