Todd Lipcon has posted comments on this change. Change subject: KUDU-1410 (part 1). Add per-Trace counters ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/2794/3//COMMIT_MSG Commit Message: Line 37: > Why not also counters for disk writes? Is it because they're harder to asso Nah I just was focusing on Scan RPCs to drive this list. I anticipate we'll probably add more going forward and this was just a starting point to get some testing / validation of usefulness. I also noticed that we currently don't create a Trace for each log sync batch, and maybe we should do so and associate it as a "related trace" to any requests that got batched into it, since the majority of write-side outliers are just due to log slowness. http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 994: TRACE_COUNTER_INCREMENT("lbm_read_time_us", dur); > No love for the file block manager? mind if I do that in a follow-up? http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/util/mutex.cc File src/kudu/util/mutex.cc: Line 73: // Optimize for the case when mutexes are uncontended. If they : // are contended, we'll have to go to sleep anyway, so the extra : // cost of branch mispredictions is moot. : if (PREDICT_TRUE(TryAcquire())) { : return; : } > Seems unrelated to the trace metrics change. Or is this because you didn't Yea, I wanted to measure wait time on all builds. I just checked and pthread trylock does avoid the syscall -- it's just a lock:cmpxchg underneath. I compiled a little test program that created a mutex and trylocked, and then stepped through with gdb: https://gist.github.com/3c6a8e121f65cb70a9bf502239eeb216 Also I've been doing lots of benchmarking of this patch series and if there's any extra contention here it's not measurable, and the extra debuggability is worth any CPU cost. http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/util/trace_metrics.h File src/kudu/util/trace_metrics.h: Line 48: static const char* InternName(const std::string& name); > The name is never generated using user data, right? Asking because only enc Right, if it's user data then you're also exposed to a leak. The DCHECK that we don't intern more than 100 unique strings also should protect us here. I'll add a DCHECK that the name is all printable chars -- To view, visit http://gerrit.cloudera.org:8080/2794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86001778976f01b62342f63f98b62962bc74212f Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
