Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
429393003:
Send idle notifications to V8 after commit (Closed)
|
Created:
6 years, 4 months ago by ernstm Modified:
6 years, 3 months ago Reviewers:
jamesr, Sami, brianderson, enne (OOO), danakj, Hannes Payer (out of office), ulan, brettw CC:
chromium-reviews, creis+watch_chromium.org, nkostylev+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, oshima+watch_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSend idle notifications to V8 after commit
Send idle notifications for V8 to do garbage collection after
the commit. Behind a flag until V8 side of implementation is
complete and tested.
R=brianderson@chromium.org,skyostil@chromium.org,hpayer@chromium.org,danakj@chromium.org
BUG=
Committed: https://crrev.com/50aec929542bb04f2c80ef4d3a1f5d3efe07750e
Cr-Commit-Position: refs/heads/master@{#291995}
Patch Set 1 #Patch Set 2 : comment #Patch Set 3 : Call idle notification from RenderWidgetCompositor. #
Total comments: 5
Patch Set 4 : Don't store flag in settings. #Patch Set 5 : Move flag from cc:switches to content switches. #
Total comments: 2
Patch Set 6 : idle_time_for_gc -> idle_time_in_ms #
Total comments: 9
Patch Set 7 : Clean-ups. #
Total comments: 4
Patch Set 8 : Clean-ups. #Patch Set 9 : rebase #
Total comments: 4
Patch Set 10 : Improve comments. #Patch Set 11 : . #
Total comments: 2
Patch Set 12 : Avoid 64-bit division. #
Messages
Total messages: 28 (0 generated)
In order to implement and test the V8 side of idle notifications for micro garbage collection, we need a source of those notifications (eventually they'll be generated by the Blink scheduler). Obviously the implementation in this patch isn't very clean. Calling into V8 directly from CC violates checkdeps, and the shared variable in ThreadProxy also looks odd. Is there a better way to implement these notifications?
On 2014/08/01 08:30:15, ernstm wrote: > In order to implement and test the V8 side of idle notifications for micro > garbage collection, we need a source of those notifications (eventually they'll > be generated by the Blink scheduler). > > Obviously the implementation in this patch isn't very clean. Calling into V8 > directly from CC violates checkdeps, and the shared variable in ThreadProxy also > looks odd. Is there a better way to implement these notifications? You'll want to do this via content/renderer/something. That's the piece that knows about both cc/ and v8.
I've got a WIP patch that communicates more BeginFrame timing information from cc to content, and ultimately to the Blink Scheduler: https://codereview.chromium.org/429743003/ Would that work for your purposes here? I'm pondering whether to do what you did here and eagerly start idle work as soon as the BeginMainFrame finishes, or whether to be more conservative and use the provided deadline to avoid stealing cycles from the renderer and browser compositors.
On 2014/08/08 11:14:27, Sami wrote: > I've got a WIP patch that communicates more BeginFrame timing information from > cc to content, and ultimately to the Blink Scheduler: > https://codereview.chromium.org/429743003/ > > Would that work for your purposes here? I'm pondering whether to do what you did > here and eagerly start idle work as soon as the BeginMainFrame finishes, or > whether to be more conservative and use the provided deadline to avoid stealing > cycles from the renderer and browser compositors. Yes, your patch is very useful for this. After discussion with brianderson@ we want to use the args from BeginMainFrame anyway. Whether to start GC after the commit or after the deadline is an open question. Starting right after the commit has the benefit of exposing more parallelism earlier, which is beneficial on devices with many cores. Not so sure about weaker devices. We should run some experiments. I've updated the patch to issue the idle notifications from RenderWidgetCompositor using the args of BeginMainFrame.
https://codereview.chromium.org/429393003/diff/40001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/429393003/diff/40001/cc/base/switches.cc#newc... cc/base/switches.cc:125: "send-v8-idle-notification-after-commit"; this switch doesn't influence any behavior in cc/, it changes the behavior of content/. it should go in content's switches https://codereview.chromium.org/429393003/diff/40001/content/renderer/gpu/ren... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/40001/content/renderer/gpu/ren... content/renderer/gpu/render_widget_compositor.cc:177: settings.send_v8_idle_notification_after_commit = you're setting this here in content/renderer/... https://codereview.chromium.org/429393003/diff/40001/content/renderer/gpu/ren... content/renderer/gpu/render_widget_compositor.cc:796: if (layer_tree_host_->settings().send_v8_idle_notification_after_commit && ... and then querying it again here, in the same object. there's no need to involve the cc:: settings object at all. just store a bool or query the flag status again here
https://codereview.chromium.org/429393003/diff/40001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/429393003/diff/40001/cc/base/switches.cc#newc... cc/base/switches.cc:125: "send-v8-idle-notification-after-commit"; On 2014/08/18 23:23:49, jamesr wrote: > this switch doesn't influence any behavior in cc/, it changes the behavior of > content/. it should go in content's switches Done. https://codereview.chromium.org/429393003/diff/40001/content/renderer/gpu/ren... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/40001/content/renderer/gpu/ren... content/renderer/gpu/render_widget_compositor.cc:796: if (layer_tree_host_->settings().send_v8_idle_notification_after_commit && On 2014/08/18 23:23:49, jamesr wrote: > ... and then querying it again here, in the same object. there's no need to > involve the cc:: settings object at all. just store a bool or query the flag > status again here Done.
https://codereview.chromium.org/429393003/diff/80001/content/renderer/gpu/ren... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/80001/content/renderer/gpu/ren... content/renderer/gpu/render_widget_compositor.cc:792: int idle_time_for_gc = Let's rename that variable to idle_time_in_ms. The embedder does not know what v8 is doing in the idle time.
https://codereview.chromium.org/429393003/diff/80001/content/renderer/gpu/ren... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/80001/content/renderer/gpu/ren... content/renderer/gpu/render_widget_compositor.cc:792: int idle_time_for_gc = On 2014/08/19 07:34:36, Hannes Payer wrote: > Let's rename that variable to idle_time_in_ms. The embedder does not know what > v8 is doing in the idle time. Done.
Awesome stuff, LGTM.
On 2014/08/20 11:27:09, Hannes Payer wrote: > Awesome stuff, LGTM. James, are you okay with this patch?
https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:791: if (cmd->HasSwitch(switches::kSendV8IdleNotificationAfterCommit)) { grab this value once and stash it in a bool, it can't change over time. HasSwitch() is a lowercase conversion (on windows) and an std::map<> lookup, so not terribly slow, but no need to bother doing this over and over when it'll always give us the same answer https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:795: base::TimeTicks::HighResNow()).InMilliseconds()); i was under the impression BeginFrameArgs was built on top of gfx::FrameTime::Now(), not base::TimeTicks::HighResNow(). Can you check this please? https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:797: blink::mainThreadIsolate()->IdleNotification(idle_time_in_ms); why the main thread isolate in particular? what about other heaps? https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:160: cc::BeginFrameArgs begin_main_frame_args_; seems wasteful to keep a full BeginFrameArgs around only to grab two values off of it. today BeginFrameArgs is pretty small, but I believe it'll get bigger and it's unlikely someone will check this usage again as it does. Can you just keep the base::Time{Delta,Ticks} you need instead?
https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:795: base::TimeTicks::HighResNow()).InMilliseconds()); On 2014/08/20 19:38:41, jamesr wrote: > i was under the impression BeginFrameArgs was built on top of > gfx::FrameTime::Now(), not base::TimeTicks::HighResNow(). Can you check this > please? James is correct. Using HighResNow() here directly will break on Windows. You should use gfx::FrameTime::Now() instead.
https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:791: if (cmd->HasSwitch(switches::kSendV8IdleNotificationAfterCommit)) { On 2014/08/20 19:38:41, jamesr wrote: > grab this value once and stash it in a bool, it can't change over time. > HasSwitch() is a lowercase conversion (on windows) and an std::map<> lookup, so > not terribly slow, but no need to bother doing this over and over when it'll > always give us the same answer Done. https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:795: base::TimeTicks::HighResNow()).InMilliseconds()); On 2014/08/20 20:07:47, brianderson wrote: > On 2014/08/20 19:38:41, jamesr wrote: > > i was under the impression BeginFrameArgs was built on top of > > gfx::FrameTime::Now(), not base::TimeTicks::HighResNow(). Can you check this > > please? > > James is correct. Using HighResNow() here directly will break on Windows. You > should use gfx::FrameTime::Now() instead. Done. https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:797: blink::mainThreadIsolate()->IdleNotification(idle_time_in_ms); On 2014/08/20 19:38:41, jamesr wrote: > why the main thread isolate in particular? what about other heaps? The goal is to reduce jank caused by GC on the main thread before the commit. Web workers already run in separate threads, so shouldn't cause problems. https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:160: cc::BeginFrameArgs begin_main_frame_args_; On 2014/08/20 19:38:41, jamesr wrote: > seems wasteful to keep a full BeginFrameArgs around only to grab two values off > of it. today BeginFrameArgs is pretty small, but I believe it'll get bigger and > it's unlikely someone will check this usage again as it does. Can you just keep > the base::Time{Delta,Ticks} you need instead? Done.
On 2014/08/21 18:22:00, ernstm wrote: > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > File content/renderer/gpu/render_widget_compositor.cc (right): > > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > content/renderer/gpu/render_widget_compositor.cc:791: if > (cmd->HasSwitch(switches::kSendV8IdleNotificationAfterCommit)) { > On 2014/08/20 19:38:41, jamesr wrote: > > grab this value once and stash it in a bool, it can't change over time. > > HasSwitch() is a lowercase conversion (on windows) and an std::map<> lookup, > so > > not terribly slow, but no need to bother doing this over and over when it'll > > always give us the same answer > > Done. > > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > content/renderer/gpu/render_widget_compositor.cc:795: > base::TimeTicks::HighResNow()).InMilliseconds()); > On 2014/08/20 20:07:47, brianderson wrote: > > On 2014/08/20 19:38:41, jamesr wrote: > > > i was under the impression BeginFrameArgs was built on top of > > > gfx::FrameTime::Now(), not base::TimeTicks::HighResNow(). Can you check > this > > > please? > > > > James is correct. Using HighResNow() here directly will break on Windows. You > > should use gfx::FrameTime::Now() instead. > > Done. > > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > content/renderer/gpu/render_widget_compositor.cc:797: > blink::mainThreadIsolate()->IdleNotification(idle_time_in_ms); > On 2014/08/20 19:38:41, jamesr wrote: > > why the main thread isolate in particular? what about other heaps? > The goal is to reduce jank caused by GC on the main thread before the commit. > Web workers already run in separate threads, so shouldn't cause problems. > > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > File content/renderer/gpu/render_widget_compositor.h (right): > > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > content/renderer/gpu/render_widget_compositor.h:160: cc::BeginFrameArgs > begin_main_frame_args_; > On 2014/08/20 19:38:41, jamesr wrote: > > seems wasteful to keep a full BeginFrameArgs around only to grab two values > off > > of it. today BeginFrameArgs is pretty small, but I believe it'll get bigger > and > > it's unlikely someone will check this usage again as it does. Can you just > keep > > the base::Time{Delta,Ticks} you need instead? > > Done. ping
On 2014/08/21 18:22:00, ernstm wrote: > https://codereview.chromium.org/429393003/diff/100001/content/renderer/gpu/re... > content/renderer/gpu/render_widget_compositor.cc:797: > blink::mainThreadIsolate()->IdleNotification(idle_time_in_ms); > On 2014/08/20 19:38:41, jamesr wrote: > > why the main thread isolate in particular? what about other heaps? > The goal is to reduce jank caused by GC on the main thread before the commit. > Web workers already run in separate threads, so shouldn't cause problems. There can be many isolates on the same thread. isolates != threads, isolates are independent heaps. Extensions currently use main thread isolates and blink-in-JS will soon as well. Seems like what you want here is a signal to V8 that the thread is going to be idle, not that a particular isolate will.
content/renderer/ lgtm, but please work with the V8 folks to improve the idle API. Targetting a specific isolate doesn't really make a lot of sense. https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:168: compositor->send_v8_idle_notification_after_commit_ = true; touching a private var directly is dodgy. make this a constructor parameter? https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:15: #include "cc/output/begin_frame_args.h" don't need
The idle notification was recently moved to the isolate: https://codereview.chromium.org/412163003/. Let's discuss with Jochen and Hannes when they are back from vacation. https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:168: compositor->send_v8_idle_notification_after_commit_ = true; On 2014/08/25 18:06:26, jamesr wrote: > touching a private var directly is dodgy. make this a constructor parameter? I moved the check for the command line flag into the constructor and set the member there. Is that okay? https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:15: #include "cc/output/begin_frame_args.h" On 2014/08/25 18:06:26, jamesr wrote: > don't need Done.
On 2014/08/25 18:25:11, ernstm wrote: > https://codereview.chromium.org/429393003/diff/120001/content/renderer/gpu/re... > content/renderer/gpu/render_widget_compositor.cc:168: > compositor->send_v8_idle_notification_after_commit_ = true; > On 2014/08/25 18:06:26, jamesr wrote: > > touching a private var directly is dodgy. make this a constructor parameter? > > I moved the check for the command line flag into the constructor and set the > member there. Is that okay? > That's fine. If we need to construct this differently in tests or something we might need to move it back out of the c'tor, but no need to speculatively optimize.
ernstm@chromium.org changed reviewers: + brettw@chromium.org
On 2014/08/26 16:49:28, ernstm wrote: > mailto:ernstm@chromium.org changed reviewers: > + mailto:brettw@chromium.org Brett, could you please take a quick look at the content/public and content/browser changes (adding a command line flag)?
lgtm https://codereview.chromium.org/429393003/diff/160001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/429393003/diff/160001/content/public/common/c... content/public/common/content_switches.cc:688: // Send idle notifications to V8 after commit. This comment literally duplicates the name of the variable so is just extra noise. This should explain what actually that means and what it's for (and probably also mention that it's an experiment while it's being worked on). https://codereview.chromium.org/429393003/diff/160001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/429393003/diff/160001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:159: base::TimeDelta begin_main_frame_interval_; The previous variable seems self-explanatory, but this one does not. Can you document this?
https://codereview.chromium.org/429393003/diff/160001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/429393003/diff/160001/content/public/common/c... content/public/common/content_switches.cc:688: // Send idle notifications to V8 after commit. On 2014/08/26 17:48:46, brettw wrote: > This comment literally duplicates the name of the variable so is just extra > noise. This should explain what actually that means and what it's for (and > probably also mention that it's an experiment while it's being worked on). Done. https://codereview.chromium.org/429393003/diff/160001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/429393003/diff/160001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.h:159: base::TimeDelta begin_main_frame_interval_; On 2014/08/26 17:48:46, brettw wrote: > The previous variable seems self-explanatory, but this one does not. Can you > document this? Done.
lgtm, just one nit regarding 64-bit division. https://codereview.chromium.org/429393003/diff/200001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/200001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:793: gfx::FrameTime::Now()).InMilliseconds()); InMilliseconds() uses 64-bit division, which I understand is emulated or very slow on some devices. It's only once per frame, so this may be completely over-optimizing, but we might as well avoid it if we can. Can you store the initial result in a base::TimeDelta and then only convert to milliseconds if that result is > base::TimeDelta(). Then, to avoid the 64-bit division, convert the TimeDelta to *microseconds* first, store that into a 32-bit value (which should be more than enough bits), and then convert to milliseconds.
https://codereview.chromium.org/429393003/diff/200001/content/renderer/gpu/re... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/429393003/diff/200001/content/renderer/gpu/re... content/renderer/gpu/render_widget_compositor.cc:793: gfx::FrameTime::Now()).InMilliseconds()); On 2014/08/26 19:05:41, brianderson wrote: > InMilliseconds() uses 64-bit division, which I understand is emulated or very > slow on some devices. It's only once per frame, so this may be completely > over-optimizing, but we might as well avoid it if we can. > > Can you store the initial result in a base::TimeDelta and then only convert to > milliseconds if that result is > base::TimeDelta(). > > Then, to avoid the 64-bit division, convert the TimeDelta to *microseconds* > first, store that into a 32-bit value (which should be more than enough bits), > and then convert to milliseconds. Done.
The CQ bit was checked by ernstm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ernstm@chromium.org/429393003/220001
Message was sent while issue was closed.
Committed patchset #12 (220001) as 32505dba1054184c41c0441637e5cc2c58c0f572
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/50aec929542bb04f2c80ef4d3a1f5d3efe07750e Cr-Commit-Position: refs/heads/master@{#291995} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
