Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1141393002:
Restore NothingOrDone action in idle time handler. (Closed)
DescriptionRestore NothingOrDone action in idle time handler.
This also adjusts transitioning between modes so that crbug.com/460090 remains fixed.
BUG=chromium:489323, chromium:460090
LOG=NO
Committed: https://crrev.com/39491c5168ee283e2a2e0053d0f6d809e1631058
Cr-Commit-Position: refs/heads/master@{#28490}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add NothingOrDone in ShouldDoContextDisposalMarkCompact #Patch Set 3 : Restore tests #Patch Set 4 : Make backround idle time counter explicit #
Total comments: 4
Patch Set 5 : Address comments from Ross #
Messages
Total messages: 22 (7 generated)
ulan@chromium.org changed reviewers: + hpayer@chromium.org, rmcilroy@chromium.org
PTAL
https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... src/heap/gc-idle-time-handler.cc:297: return GCIdleTimeAction::Nothing(); What about this one and the one above? Also nothing or done?
https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... src/heap/gc-idle-time-handler.cc:297: return GCIdleTimeAction::Nothing(); On 2015/05/19 09:50:32, Hannes Payer wrote: > What about this one and the one above? Also nothing or done? Replaced this one with NothingOrDone. Leaving the one above as Nothing() like it was before refactoring.
https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... src/heap/gc-idle-time-handler.cc:297: return GCIdleTimeAction::Nothing(); On 2015/05/19 09:50:32, Hannes Payer wrote: > What about this one and the one above? Also nothing or done? Just this one I think (the one above should probably still be Nothing since it only happens if we get a negative idle time). https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... src/heap/gc-idle-time-handler.cc:348: kLongIdleNotificationsBeforeMutatorIsIdle / 2; I don't understand this - what is kLongIdleNotificationsBeforeMutatorIsIdle representing and why do we divide it by two here? https://codereview.chromium.org/1141393002/diff/1/test/unittests/heap/gc-idle... File test/unittests/heap/gc-idle-time-handler-unittest.cc (right): https://codereview.chromium.org/1141393002/diff/1/test/unittests/heap/gc-idle... test/unittests/heap/gc-idle-time-handler-unittest.cc:661: Can we add back the two tests for NothingOrDone which were removed in the previous CL too please.
https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... src/heap/gc-idle-time-handler.cc:348: kLongIdleNotificationsBeforeMutatorIsIdle / 2; On 2015/05/19 10:26:57, rmcilroy wrote: > I don't understand this - what is kLongIdleNotificationsBeforeMutatorIsIdle > representing and why do we divide it by two here? This was trying to encode two different counters: one for foreground tab and one for background tab. For background tab we want to go faster in reduce memory mode. I made it explicit in the new patch set. https://codereview.chromium.org/1141393002/diff/1/test/unittests/heap/gc-idle... File test/unittests/heap/gc-idle-time-handler-unittest.cc (right): https://codereview.chromium.org/1141393002/diff/1/test/unittests/heap/gc-idle... test/unittests/heap/gc-idle-time-handler-unittest.cc:661: On 2015/05/19 10:26:57, rmcilroy wrote: > Can we add back the two tests for NothingOrDone which were removed in the > previous CL too please. Done.
lgtm, thanks. https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... src/heap/gc-idle-time-handler.cc:348: kLongIdleNotificationsBeforeMutatorIsIdle / 2; On 2015/05/19 11:21:52, ulan wrote: > On 2015/05/19 10:26:57, rmcilroy wrote: > > I don't understand this - what is kLongIdleNotificationsBeforeMutatorIsIdle > > representing and why do we divide it by two here? > > This was trying to encode two different counters: one for foreground tab and one > for background tab. For background tab we want to go faster in reduce memory > mode. I made it explicit in the new patch set. This is clearer, thanks. https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... src/heap/gc-idle-time-handler.cc:351: background_idle_notifications_++; do we want this instead: if (idle_time_in_ms >= kMinBackgroundIdleTime) { background_idle_notifications_++; } else if (idle_time_in_ms >= kMinLongIdleTime) { long_idle_notifications_++; } This would ensure we don't overcount long_idle_notifications. https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... src/heap/gc-idle-time-handler.cc:376: int gcs) { nit - gcs -> mutator_gcs
On 2015/05/19 14:36:34, rmcilroy wrote: > lgtm, thanks. > > https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... > File src/heap/gc-idle-time-handler.cc (right): > > https://codereview.chromium.org/1141393002/diff/1/src/heap/gc-idle-time-handl... > src/heap/gc-idle-time-handler.cc:348: kLongIdleNotificationsBeforeMutatorIsIdle > / 2; > On 2015/05/19 11:21:52, ulan wrote: > > On 2015/05/19 10:26:57, rmcilroy wrote: > > > I don't understand this - what is kLongIdleNotificationsBeforeMutatorIsIdle > > > representing and why do we divide it by two here? > > > > This was trying to encode two different counters: one for foreground tab and > one > > for background tab. For background tab we want to go faster in reduce memory > > mode. I made it explicit in the new patch set. > > This is clearer, thanks. > > https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... > File src/heap/gc-idle-time-handler.cc (right): > > https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... > src/heap/gc-idle-time-handler.cc:351: background_idle_notifications_++; > do we want this instead: > > if (idle_time_in_ms >= kMinBackgroundIdleTime) { > background_idle_notifications_++; > } else if (idle_time_in_ms >= kMinLongIdleTime) { > long_idle_notifications_++; > } > > This would ensure we don't overcount long_idle_notifications. > > https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... > src/heap/gc-idle-time-handler.cc:376: int gcs) { > nit - gcs -> mutator_gcs Lgtm
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1141393002/#ps80001 (title: "Address comments from Ross")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141393002/80001
Thank, landing https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... File src/heap/gc-idle-time-handler.cc (right): https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... src/heap/gc-idle-time-handler.cc:351: background_idle_notifications_++; On 2015/05/19 14:36:33, rmcilroy wrote: > do we want this instead: > > if (idle_time_in_ms >= kMinBackgroundIdleTime) { > background_idle_notifications_++; > } else if (idle_time_in_ms >= kMinLongIdleTime) { > long_idle_notifications_++; > } > > This would ensure we don't overcount long_idle_notifications. Done. https://codereview.chromium.org/1141393002/diff/60001/src/heap/gc-idle-time-h... src/heap/gc-idle-time-handler.cc:376: int gcs) { On 2015/05/19 14:36:33, rmcilroy wrote: > nit - gcs -> mutator_gcs Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The CQ bit was checked by ulan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141393002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/4310)
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/1141393002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/39491c5168ee283e2a2e0053d0f6d809e1631058 Cr-Commit-Position: refs/heads/master@{#28490} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
