Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1614063005:
Adds a callback to TracingAgent::StartAgentTracing() (Closed)
|
Created:
4 years, 11 months ago by charliea (OOO until 10-5) Modified:
4 years, 10 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, jam, darin-cc_chromium.org, oshima+watch_chromium.org, tracing+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a callback to TracingAgent::StartAgentTracing()
This allows us to use tracing agents that require more time to start.
The first use case of this will be the BattOr tracing agent, which
requires several round trips over USB before it can know whether
StartTracing was successful.
Committed: https://crrev.com/c1b44047e693b68a9a441a1d9f464e3c9e16ed89
Cr-Commit-Position: refs/heads/master@{#371637}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Code review #
Total comments: 2
Patch Set 3 : #
Total comments: 12
Patch Set 4 : Code review #Patch Set 5 : Synced to head #Patch Set 6 : #Messages
Total messages: 67 (34 generated)
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/80001
The CQ bit was unchecked by charliea@chromium.org
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
charliea@chromium.org changed reviewers: + zhenw@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/100001
https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... File content/browser/tracing/power_tracing_agent.cc (right): https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/power_tracing_agent.cc:50: if (!battor_trace_provider_->IsConnected()) { This is still a sync call here. Will this be updated to async call in later CL? https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:227: pending_start_tracing_ack_count_ = 1; nit: Maybe it is better to move the count for Chrome tracing agent to where it is called (line 264-266), so that it is consistent with other agents? https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:269: if (!start_tracing_done_callback_.is_null()) { I think we always want to have the timer. start_tracing_done_callback_ is used to notify clients, e.g., the tracing UI. It is not related to whether a agent starts tracing in 30 sec. https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:705: OnAllTracingAgentsStarted(); Should also stop start_tracing_timer_ here.
charliea@chromium.org changed reviewers: + oysteine@chromium.org
https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... File content/browser/tracing/power_tracing_agent.cc (right): https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/power_tracing_agent.cc:50: if (!battor_trace_provider_->IsConnected()) { On 2016/01/25 20:35:32, Zhen Wang wrote: > This is still a sync call here. Will this be updated to async call in later CL? All interaction with the serial classes needs to be done on the IO thread. However, the IsConnected() call, once on the IO thread, is a synchronous call. I think that we're going to have 3 main StartTracing functions in this class all together: StartTracing (on UI thread), StartTracingOnIOThread, and IsConnected(). StartTracing will defer to StartTracingOnIOThread, which will in turn call IsConnected. That was really complicated to explain, but if it doesn't make sense, I think it should be a lot more clear w/ the next CL. https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:227: pending_start_tracing_ack_count_ = 1; On 2016/01/25 20:35:32, Zhen Wang wrote: > nit: Maybe it is better to move the count for Chrome tracing agent to where it > is called (line 264-266), so that it is consistent with other agents? Done. https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:269: if (!start_tracing_done_callback_.is_null()) { On 2016/01/25 20:35:32, Zhen Wang wrote: > I think we always want to have the timer. > > start_tracing_done_callback_ is used to notify clients, e.g., the tracing UI. It > is not related to whether a agent starts tracing in 30 sec. Thanks for catching that! This is left-over from when I was (mistakenly) directly posting start_tracing_done_callback_ from here and bypassing OnAllTracingAgentsStarted. https://codereview.chromium.org/1614063005/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:705: OnAllTracingAgentsStarted(); On 2016/01/25 20:35:32, Zhen Wang wrote: > Should also stop start_tracing_timer_ here. Doh. Done.
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:690: void TracingControllerImpl::OnStartAgentTracingAcked( What happens if StopTracing() gets called before all the callbacks have been called? Looks like we may be getting in some inconsistent state at that point; may need to insert a check, though I could be wrong.
https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:690: void TracingControllerImpl::OnStartAgentTracingAcked( On 2016/01/25 22:19:41, Oystein wrote: > What happens if StopTracing() gets called before all the callbacks have been > called? Looks like we may be getting in some inconsistent state at that point; > may need to insert a check, though I could be wrong. One thing that we could do is, when StopTracing is called, do something like: if (start_tracing_timer_.IsRunning()) { start_tracing_timer_.Stop(); OnAllTracingAgentsStarted(); PostTask(BrowserThread::UI, base::Bind(&StopTracing, base::Unretained(this), trace_data_sink)); } In other words, if we see that we're still waiting for tracing agents, immediately stop the timer to signal that the window has closed for agents to register themselves, signal that all tracing agents are started to do any necessary cleanup, and then retry StopTracing. I could see this could be problematic if the StopTracing code relied on posted tasks from within OnAllTracingAgentsStarted to be run, though, because I don't think the order of execution of tasks is guaranteed within task runners. That's the most reasonable thing I can think of. WDYT?
On 2016/01/26 00:30:09, charliea wrote: > https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... > File content/browser/tracing/tracing_controller_impl.cc (right): > > https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... > content/browser/tracing/tracing_controller_impl.cc:690: void > TracingControllerImpl::OnStartAgentTracingAcked( > On 2016/01/25 22:19:41, Oystein wrote: > > What happens if StopTracing() gets called before all the callbacks have been > > called? Looks like we may be getting in some inconsistent state at that point; > > may need to insert a check, though I could be wrong. > > One thing that we could do is, when StopTracing is called, do something like: > > if (start_tracing_timer_.IsRunning()) { > start_tracing_timer_.Stop(); > OnAllTracingAgentsStarted(); > PostTask(BrowserThread::UI, base::Bind(&StopTracing, base::Unretained(this), > trace_data_sink)); > } > > In other words, if we see that we're still waiting for tracing agents, > immediately stop the timer to signal that the window has closed for agents to > register themselves, signal that all tracing agents are started to do any > necessary cleanup, and then retry StopTracing. I could see this could be > problematic if the StopTracing code relied on posted tasks from within > OnAllTracingAgentsStarted to be run, though, because I don't think the order of > execution of tasks is guaranteed within task runners. That's the most reasonable > thing I can think of. WDYT? Retrying StopTracing sounds pretty reasonable to me, but yeah I'm a bit worried about the thread hopping as well. The agents could definitely have tasks being posted to FILE/IO threads and then we might get some hard to foresee issues here. Maybe an even simpler version of what you posted: if the start_tracing_timer_ is still running, post the StopTracing task and return. Rince and repeat.
On 2016/01/26 01:06:52, Oystein wrote: > On 2016/01/26 00:30:09, charliea wrote: > > > https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... > > File content/browser/tracing/tracing_controller_impl.cc (right): > > > > > https://codereview.chromium.org/1614063005/diff/120001/content/browser/tracin... > > content/browser/tracing/tracing_controller_impl.cc:690: void > > TracingControllerImpl::OnStartAgentTracingAcked( > > On 2016/01/25 22:19:41, Oystein wrote: > > > What happens if StopTracing() gets called before all the callbacks have been > > > called? Looks like we may be getting in some inconsistent state at that > point; > > > may need to insert a check, though I could be wrong. > > > > One thing that we could do is, when StopTracing is called, do something like: > > > > if (start_tracing_timer_.IsRunning()) { > > start_tracing_timer_.Stop(); > > OnAllTracingAgentsStarted(); > > PostTask(BrowserThread::UI, base::Bind(&StopTracing, base::Unretained(this), > > trace_data_sink)); > > } > > > > In other words, if we see that we're still waiting for tracing agents, > > immediately stop the timer to signal that the window has closed for agents to > > register themselves, signal that all tracing agents are started to do any > > necessary cleanup, and then retry StopTracing. I could see this could be > > problematic if the StopTracing code relied on posted tasks from within > > OnAllTracingAgentsStarted to be run, though, because I don't think the order > of > > execution of tasks is guaranteed within task runners. That's the most > reasonable > > thing I can think of. WDYT? > > Retrying StopTracing sounds pretty reasonable to me, but yeah I'm a bit worried > about the thread hopping as well. The agents could definitely have tasks being > posted to FILE/IO threads and then we might get some hard to foresee issues > here. Maybe an even simpler version of what you posted: if the > start_tracing_timer_ is still running, post the StopTracing task and return. > Rince and repeat. That works (and I agree - I like it more, and it's conceptually much simpler). I'll do that.
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/150001
PTAL
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Patchset #3 (id:150001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/170001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #3 (id:170001) has been deleted
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/190001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, lgtm!
charliea@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb for chromeos OWNERS
https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:226: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); Document |success|, i.e: true /* success */ https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/fake_deb... File chromeos/dbus/fake_debug_daemon_client.cc (right): https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/fake_deb... chromeos/dbus/fake_debug_daemon_client.cc:64: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); /* success */ https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... File content/browser/tracing/etw_system_event_consumer_win.cc (right): https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/etw_system_event_consumer_win.cc:59: FROM_HERE, base::Bind(callback, GetTracingAgentName(), false)); /* success */ https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/etw_system_event_consumer_win.cc:71: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); /* success */ https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... File content/browser/tracing/power_tracing_agent.cc (right): https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/power_tracing_agent.cc:52: FROM_HERE, base::Bind(callback, GetTracingAgentName(), false)); /* success */ https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/power_tracing_agent.cc:61: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); /* success */
https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:226: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); On 2016/01/26 20:11:19, stevenjb wrote: > Document |success|, i.e: true /* success */ Ah, didn't know that you were supposed to do this. Done. https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/fake_deb... File chromeos/dbus/fake_debug_daemon_client.cc (right): https://codereview.chromium.org/1614063005/diff/190001/chromeos/dbus/fake_deb... chromeos/dbus/fake_debug_daemon_client.cc:64: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); On 2016/01/26 20:11:19, stevenjb wrote: > /* success */ Done. https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... File content/browser/tracing/etw_system_event_consumer_win.cc (right): https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/etw_system_event_consumer_win.cc:59: FROM_HERE, base::Bind(callback, GetTracingAgentName(), false)); On 2016/01/26 20:11:19, stevenjb wrote: > /* success */ Done. https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/etw_system_event_consumer_win.cc:71: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); On 2016/01/26 20:11:19, stevenjb wrote: > /* success */ Done. https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... File content/browser/tracing/power_tracing_agent.cc (right): https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/power_tracing_agent.cc:52: FROM_HERE, base::Bind(callback, GetTracingAgentName(), false)); On 2016/01/26 20:11:20, stevenjb wrote: > /* success */ Done. https://codereview.chromium.org/1614063005/diff/190001/content/browser/tracin... content/browser/tracing/power_tracing_agent.cc:61: FROM_HERE, base::Bind(callback, GetTracingAgentName(), true)); On 2016/01/26 20:11:20, stevenjb wrote: > /* success */ Done.
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/210001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1614063005/#ps230001 (title: "Synced to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:250001) has been deleted
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1614063005/#ps270001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1614063005/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1614063005/270001
Message was sent while issue was closed.
Committed patchset #6 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Adds a callback to TracingAgent::StartAgentTracing() This allows us to use tracing agents that require more time to start. The first use case of this will be the BattOr tracing agent, which requires several round trips over USB before it can know whether StartTracing was successful. ========== to ========== Adds a callback to TracingAgent::StartAgentTracing() This allows us to use tracing agents that require more time to start. The first use case of this will be the BattOr tracing agent, which requires several round trips over USB before it can know whether StartTracing was successful. Committed: https://crrev.com/c1b44047e693b68a9a441a1d9f464e3c9e16ed89 Cr-Commit-Position: refs/heads/master@{#371637} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c1b44047e693b68a9a441a1d9f464e3c9e16ed89 Cr-Commit-Position: refs/heads/master@{#371637} |
