Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891973003:
tracing: the agent registry implementation (Closed)
|
Created:
3 years, 7 months ago by chiniforooshan Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, chrome-grc-reviews_chromium.org, chromium-reviews, darin (slow to review), Primiano Tucci (use gerrit), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing: the agent registry implementation
The agent registry keeps track of all tracing agents. The coordinator
communicates to the agents through the agent registry.
A prototype of the final product (a little bit stale and incomplete):
https://codereview.chromium.org/2833873003/
Design doc (use @chromium.org account):
https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM
BUG=640235
Review-Url: https://codereview.chromium.org/2891973003
Cr-Commit-Position: refs/heads/master@{#475138}
Committed: https://chromium.googlesource.com/chromium/src/+/536bfe1a8b9e601815f090e138e02de817a99df5
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : use thread checker macros #
Total comments: 32
Patch Set 4 : review + rebase #Patch Set 5 : one more DCHECK #Patch Set 6 : review #
Messages
Total messages: 23 (11 generated)
chiniforooshan@chromium.org changed reviewers: + kenrb@chromium.org, oysteine@chromium.org
ptal
The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/19 03:00:47, chiniforooshan wrote: > ptal pinging Oystein :)
primiano@chromium.org changed reviewers: + primiano@chromium.org
Glad to see this coming along https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.cc (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:29: DCHECK(label.size()); !label.empty() might be slightly more readable https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:64: g_agent_registry = nullptr; maybe add a comment saying // for testing only (is it?), otherwise I'd argue this is racy :) https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.h (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:56: typedef base::RepeatingCallback<void(Entry*)> AgentInitializationCallback; using AgentInitializationCallback = base.... https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:58: // The getter of the unique instance. I guess this is a quite common pattern in chrome so this comment might be unnecessary https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:72: DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); is this really needed? It feels that without this you could just write below: for(const auto& agent : agents_) agent_initialization_callback_.Run(agent); so not sure this extra complexity is really helping. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:84: // mojom::AgentRegistry similar comment to the other CL. not sure if there is a suggested pattern here, but I find slightly more readable having these in the public: section as this is more the real API of the class https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:94: std::unordered_map<size_t, std::unique_ptr<Entry>> agents_; see brett's PSA on chromium-dev about unordered_map. TL;DR (IIRC) use either map<> or flat_map<>
lgtm and thanks! (and sorry for the delay again, sigh) https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.cc (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:34: AgentRegistry::Entry::~Entry() {} nit: = default; https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:37: DCHECK(closure_.is_null()); nit: "base/logging.h" https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:44: return closure_was_set; just curious: why is this needing to be returned? https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:49: std::move(closure_).Run(); This line confuses me :). Is this moving closure_ to a stack unique_ptr basically? Maybe that should be a little bit more explicit. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.h (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:16: class AgentRegistry : public mojom::AgentRegistry { s/AgentRegistry/AgentRegistryImpl/ as in the other CL seems to be the convention. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:18: class Entry { Any more verbose names that can be used for this? AgentEntry? https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:99: DISALLOW_COPY_AND_ASSIGN(AgentRegistry); nit: "base/macros.h" https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry_unittest.cc (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry_unittest.cc:18: ~MockAgent() override {} I think this is redundant? Since the class is wholly defined in the .cc anyway (and IIRC the default generated destructor here will be virtual anyway).
https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.cc (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:29: DCHECK(label.size()); On 2017/05/25 13:24:28, Primiano Tucci wrote: > !label.empty() might be slightly more readable Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:34: AgentRegistry::Entry::~Entry() {} On 2017/05/25 17:16:08, oystein wrote: > nit: = default; Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:37: DCHECK(closure_.is_null()); On 2017/05/25 17:16:08, oystein wrote: > nit: "base/logging.h" Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:44: return closure_was_set; On 2017/05/25 17:16:08, oystein wrote: > just curious: why is this needing to be returned? Coordinator will use disconnect closures to make sure that if it's waiting to receive replies from all agents, and an agent disconnects before replying, it does not wait for it forever. For example, // Send a message to an agent. agent->SOME_INTERFACE_METHOD_WITH_REPLY(..., base::Bind(&OnReply, ...)); // Set the disconnect closure. agent->SetDisconnectClosure(base::Bind(&OnReply, ...)); Then, I believe it is technically possible that |OnReply| is invoked two times if the agent replies and quickly disconnects, before the coordinator thread is freed to process the reply task. So, |OnReply| should clear the disconnect closure and bail out if it is already cleared, because that indicates that |OnReply| was already invoked for this agent. Does this make sense? https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:49: std::move(closure_).Run(); On 2017/05/25 17:16:08, oystein wrote: > This line confuses me :). Is this moving closure_ to a stack unique_ptr > basically? Maybe that should be a little bit more explicit. It marks |closure_| as movable so that the version of Run that takes rvalue reference can be selected. If I don't mark it as movable the Run that takes const reference will be selected, which is for repeating callbacks, not once callbacks. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:64: g_agent_registry = nullptr; On 2017/05/25 13:24:28, Primiano Tucci wrote: > maybe add a comment saying // for testing only (is it?), otherwise I'd argue > this is racy :) Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.h (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:16: class AgentRegistry : public mojom::AgentRegistry { On 2017/05/25 17:16:08, oystein wrote: > s/AgentRegistry/AgentRegistryImpl/ as in the other CL seems to be the > convention. As Ken mentioned in the other CL, looks like AgentRegistry is preferred :) https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:18: class Entry { On 2017/05/25 17:16:08, oystein wrote: > Any more verbose names that can be used for this? AgentEntry? Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:56: typedef base::RepeatingCallback<void(Entry*)> AgentInitializationCallback; On 2017/05/25 13:24:28, Primiano Tucci wrote: > using AgentInitializationCallback = base.... Done (I promise to remember this next time :p). https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:58: // The getter of the unique instance. On 2017/05/25 13:24:28, Primiano Tucci wrote: > I guess this is a quite common pattern in chrome so this comment might be > unnecessary Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:72: DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); On 2017/05/25 13:24:28, Primiano Tucci wrote: > is this really needed? It feels that without this you could just write below: > > for(const auto& agent : agents_) > agent_initialization_callback_.Run(agent); > > so not sure this extra complexity is really helping. This is the public method that gives access to the set of registered agents. |agents_| is private. The coordinator, that will come in the next CL, will use this for sending messages to agents (you can see the usage in the prototype, too). https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:84: // mojom::AgentRegistry On 2017/05/25 13:24:28, Primiano Tucci wrote: > similar comment to the other CL. not sure if there is a suggested pattern here, > but I find slightly more readable having these in the public: section as this is > more the real API of the class My preference is to keep this private so that only mojo internals call it from the right thread, if it's OK with you? https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:94: std::unordered_map<size_t, std::unique_ptr<Entry>> agents_; On 2017/05/25 13:24:28, Primiano Tucci wrote: > see brett's PSA on chromium-dev about unordered_map. > TL;DR (IIRC) use either map<> or flat_map<> Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.h:99: DISALLOW_COPY_AND_ASSIGN(AgentRegistry); On 2017/05/25 17:16:08, oystein wrote: > nit: "base/macros.h" Done. https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry_unittest.cc (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry_unittest.cc:18: ~MockAgent() override {} On 2017/05/25 17:16:08, oystein wrote: > I think this is redundant? Since the class is wholly defined in the .cc anyway > (and IIRC the default generated destructor here will be virtual anyway). Done.
lgtm
https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... File services/resource_coordinator/tracing/agent_registry.cc (right): https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:44: return closure_was_set; On 2017/05/26 at 15:46:31, chiniforooshan wrote: > On 2017/05/25 17:16:08, oystein wrote: > > just curious: why is this needing to be returned? > > Coordinator will use disconnect closures to make sure that if it's waiting to receive replies from all agents, and an agent disconnects before replying, it does not wait for it forever. For example, > > // Send a message to an agent. > agent->SOME_INTERFACE_METHOD_WITH_REPLY(..., base::Bind(&OnReply, ...)); > // Set the disconnect closure. > agent->SetDisconnectClosure(base::Bind(&OnReply, ...)); > > Then, I believe it is technically possible that |OnReply| is invoked two times if the agent replies and quickly disconnects, before the coordinator thread is freed to process the reply task. So, |OnReply| should clear the disconnect closure and bail out if it is already cleared, because that indicates that |OnReply| was already invoked for this agent. > > Does this make sense? Yep absolutely, thanks for the explanation! https://codereview.chromium.org/2891973003/diff/40001/services/resource_coord... services/resource_coordinator/tracing/agent_registry.cc:49: std::move(closure_).Run(); On 2017/05/26 at 15:46:31, chiniforooshan wrote: > On 2017/05/25 17:16:08, oystein wrote: > > This line confuses me :). Is this moving closure_ to a stack unique_ptr > > basically? Maybe that should be a little bit more explicit. > > It marks |closure_| as movable so that the version of Run that takes rvalue reference can be selected. If I don't mark it as movable the Run that takes const reference will be selected, which is for repeating callbacks, not once callbacks. Ouch, that's obscure. If there's no way to write this that makes this clearer, that definitely needs a comment.
> > > This line confuses me :). Is this moving closure_ to a stack unique_ptr > > > basically? Maybe that should be a little bit more explicit. > > > > It marks |closure_| as movable so that the version of Run that takes rvalue > reference can be selected. If I don't mark it as movable the Run that takes > const reference will be selected, which is for repeating callbacks, not once > callbacks. > > Ouch, that's obscure. If there's no way to write this that makes this clearer, > that definitely needs a comment. Done. Note that this pattern is used 500+ times in the code base: https://cs.chromium.org/search/?q=std::move%5C(.*%5C)%5C.Run+package:%5Echrom...
mojo lgtm, and sorry for taking so long to get to this.
On 2017/05/26 19:51:47, kenrb wrote: > mojo lgtm, and sorry for taking so long to get to this. Thanks all!
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2891973003/#ps100001 (title: "review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495828352938250,
"parent_rev": "c8981d9b925bd5efcafeb9f4b804503967ccd65e", "commit_rev":
"3766bd09d60bec7892bd04cfc77e930e5b055547"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495828352938250,
"parent_rev": "316232ae41d37cfe518010237b3f11225740ae8a", "commit_rev":
"536bfe1a8b9e601815f090e138e02de817a99df5"}
Message was sent while issue was closed.
Description was changed from ========== tracing: the agent registry implementation The agent registry keeps track of all tracing agents. The coordinator communicates to the agents through the agent registry. A prototype of the final product (a little bit stale and incomplete): https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 ========== to ========== tracing: the agent registry implementation The agent registry keeps track of all tracing agents. The coordinator communicates to the agents through the agent registry. A prototype of the final product (a little bit stale and incomplete): https://codereview.chromium.org/2833873003/ Design doc (use @chromium.org account): https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM BUG=640235 Review-Url: https://codereview.chromium.org/2891973003 Cr-Commit-Position: refs/heads/master@{#475138} Committed: https://chromium.googlesource.com/chromium/src/+/536bfe1a8b9e601815f090e138e0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/536bfe1a8b9e601815f090e138e0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
