[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 15 Feb 2018 21:58:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id, and also stores the name and system thread id of its parent. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record the name (and a system thread id) of their parent, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Reviewed-on: http://gerrit.cloudera.org:8080/9053 Reviewed-by: Dan Hecht Tested-by: Impala Public Jenkins --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 75 insertions(+), 25 deletions(-) Approvals: Dan Hecht: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1942/ -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 15 Feb 2018 18:14:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 15 Feb 2018 18:14:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 15 Feb 2018 18:14:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 7: Fixed the clang-tidy warnings. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 15 Feb 2018 13:28:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Hello Lars Volker, Tim Armstrong, Dan Hecht, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9053 to look at the new patch set (#7). Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id, and also stores the name and system thread id of its parent. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record the name (and a system thread id) of their parent, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 75 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/9053/7 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 6: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1938/ -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 15 Feb 2018 00:56:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 6: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1938/ -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 14 Feb 2018 21:11:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 14 Feb 2018 21:11:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 5: Code-Review+2 Thanks, I think this makes sense. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 14 Feb 2018 21:10:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Hello Lars Volker, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9053 to look at the new patch set (#5). Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id, and also stores the name and system thread id of its parent. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record the name (and a system thread id) of their parent, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 74 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/9053/5 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@115 PS4, Line 115: system_thread_id_ > Are you saying that thread_debug_info_ pointer may point to stray > memory? Yes, I think that can potentially happen. > If that's the case, maybe we shouldn't keep that pointer > but instead have a way to traverse TDIs and find it if still > exists? We can use the system thread id to find the parent thread and then its TDI if it still exists. I also added a copy of the parent thread name. Even if the parent thread exits, at least we will now what was its name. We can also check if parent thread name equals to the TDI's thread name that we found via the system thread id. If they're not the same, we'll know that the parent thread was re-cycled in some way. http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@119 PS4, Line 119: boost::thread::id boost_thread_id_; : int64_t system_thread_id_ = 0; > I'm not sure what you mean by "switch to the parent thread > quickly". I was just thinking of a GDB session. GDB prints out the system thread id for each thread, so we can traverse the threads quickly if we know the ID. It can be also useful if we write a script for that and we don't want to iterate through all the threads. > I don't think I fully understand the tradeoffs. Just seems > confusing to have multiple IDs and I don't understand how each of > these fields helps in debugging. Perhaps some comments attached to > them explaining how one can use them for debugging would help? I removed boost::thread::id, and added the parent's thread name. I think it is more clear, and knowing the parent thread name can be useful on its own. And we can also use it to verify the parent thread. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 14 Feb 2018 17:32:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@115 PS4, Line 115: system_thread_id_ > I think it is possible that the parent TDI object gets destroyed by the tim Are you saying that thread_debug_info_ pointer may point to stray memory? If that's the case, maybe we shouldn't keep that pointer but instead have a way to traverse TDIs and find it if still exists? http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@119 PS4, Line 119: boost::thread::id boost_thread_id_; : int64_t system_thread_id_ = 0; > boost::thread::id is less prone to being recycled, but I think this propert I'm not sure what you mean by "switch to the parent thread quickly". I don't think I fully understand the tradeoffs. Just seems confusing to have multiple IDs and I don't understand how each of these fields helps in debugging. Perhaps some comments attached to them explaining how one can use them for debugging would help? -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 12 Feb 2018 19:45:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@115 PS4, Line 115: system_thread_id_ > if that matches thread_debug_info_->system_thread_id_, why store it here al I think it is possible that the parent TDI object gets destroyed by the time the minidump is generated, but the parent thread is still running. Maybe the parent thread is a member of a thread pool, and it didn't wait for its child thread to finish and now it is already working on something else. From the system thread id at least we'll know which thread pool created the child thread. http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@119 PS4, Line 119: boost::thread::id boost_thread_id_; : int64_t system_thread_id_ = 0; > are these related? are they both needed for debugging or is it possible to boost::thread::id is less prone to being recycled, but I think this property doesn't add much value currently. If we had stored the boost::thread::id in ParentInfo, then we could have some use of it. E.g. if the parent TDI is invalid we could use the system thread id to switch to the parent thread quickly (if it's still running), then with some luck we can check the boost thread id (maybe with the help of a new TDI), and from that we could tell if this thread is really the same thread that created the child. So, we have two options here, delete the use of boost::thread::id, or use it more extensively by adding it to ParentInfo as well. Do you have a preference? -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 12 Feb 2018 13:35:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@115 PS4, Line 115: system_thread_id_ if that matches thread_debug_info_->system_thread_id_, why store it here also? http://gerrit.cloudera.org:8080/#/c/9053/4/be/src/common/thread-debug-info.h@119 PS4, Line 119: boost::thread::id boost_thread_id_; : int64_t system_thread_id_ = 0; are these related? are they both needed for debugging or is it possible to standardize on one? -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 09 Feb 2018 21:47:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 4: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h@114 PS3, Line 114: const ThreadDebugInfo* thread_debug_info_ = nullptr; > Could you elaborate more on that? You're right, I overlooked that the strings have a fixed length. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 09 Feb 2018 17:00:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Hello Lars Volker, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9053 to look at the new patch set (#4). Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id, and also stores a pointer and system thread id to its parent. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record a pointer (and a system thread id) to their parent, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 82 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/9053/4 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 3: (4 comments) thanks! http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@70 PS3, Line 70: S > nit: lowercase S woops :) Done. http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@72 PS3, Line 72: // Child should set its parent_thread_name_ to the parent's thread_name_ > Stale comment? yes, deleted it and added comments about the new behavior http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@75 PS3, Line 75: parent > parent_name for consistency with parent_* below? That would make the EXPECT Renamed it to parent_name. Also renamed 'child' below to 'child_name'. http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h@114 PS3, Line 114: const ThreadDebugInfo* thread_debug_info_ = nullptr; > Nit: I'm not sure how helpful it would be, but you could order everything i Could you elaborate more on that? To my understanding all data member has fix width. However, we could think of the fix-sized arrays in a way that they contain variable-length data. But, they are (instance_id_ and thread_name_) already at the end of the TDI class. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 09 Feb 2018 16:47:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 3: (4 comments) Mostly looks good to me, had a few minor comments. http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc File be/src/common/thread-debug-info-test.cc: http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@70 PS3, Line 70: S nit: lowercase S http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@72 PS3, Line 72: // Child should set its parent_thread_name_ to the parent's thread_name_ Stale comment? http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info-test.cc@75 PS3, Line 75: parent parent_name for consistency with parent_* below? That would make the EXPECT_EQ easier to read, too. http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/3/be/src/common/thread-debug-info.h@114 PS3, Line 114: const ThreadDebugInfo* thread_debug_info_ = nullptr; Nit: I'm not sure how helpful it would be, but you could order everything in this class and the parent to have fixed width data come before variable length data. That way it is possible to pry it out of the binary with a debugger. Feel free to ignore if you think that's unnecessary. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 07 Feb 2018 18:29:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@79 PS2, Line 79: ExtractInfoFromParent > Extract... reads like something will be returned. How about "SetParentInfo" yeah I like that. Done. http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@80 PS2, Line 80: if (parent != nullptr) { > We usually early return instead of scoping the whole function, i.e. if (par Done http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@93 PS2, Line 93: t > nit: capital T Done http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@121 PS2, Line 121: struct ParentTdi { > Can you add a comment to this struct, too? The test has a ThreadDebugInfo p Added comment, and renamed it to "ParentInfo". http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@124 PS2, Line 124: char thread_name_[THREAD_NAME_SIZE] = {}; > I wonder if we can streamline the selective duplication of the parent threa I chose the first alternative, i.e. to store the system thread id and a pointer to the parent TDI. If it is a common case that the parent exits, we can switch to the second alternative, but also adding a pointer to the parent for convenience. http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@128 PS2, Line 128: > nit: I think this newline and the next one don't add much to readability. Done http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/util/thread.h File be/src/util/thread.h: http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/util/thread.h@191 PS2, Line 191: const ThreadDebugInfo* parent_thread_info > This should go to the front (behind category or functor) now since it's str oh I see. I placed it behind functor. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 02 Feb 2018 14:38:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Hello Lars Volker, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9053 to look at the new patch set (#3). Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id and also saves its parent's name. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record their parent's name, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 81 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/9053/3 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@79 PS2, Line 79: ExtractInfoFromParent Extract... reads like something will be returned. How about "SetParentInfo"? http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@80 PS2, Line 80: if (parent != nullptr) { We usually early return instead of scoping the whole function, i.e. if (parent == nullptr) return; http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@93 PS2, Line 93: t nit: capital T http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@121 PS2, Line 121: struct ParentTdi { Can you add a comment to this struct, too? The test has a ThreadDebugInfo parent_tdi. Maybe "ParentThreadInfo" or "ParentInfo" would be more clear? http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@124 PS2, Line 124: char thread_name_[THREAD_NAME_SIZE] = {}; I wonder if we can streamline the selective duplication of the parent thread's data here. Could we instead store two things: a numerical ID for minidump based analysis that allows to find the next thread to inspect. This would likely be the system thread id. Secondly we could store a pointer to the parent thread's info. However, that would break if the parent exits before the child. As an alternative, can we wrap the relevant fields in some internal struct and then have two members, "S thread_info;" and "S parent_info;"? This way we keep all data twice, but we know for sure that we can traverse the thread ancestry two levels deep, even if the parent has exited. http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/common/thread-debug-info.h@128 PS2, Line 128: nit: I think this newline and the next one don't add much to readability. http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/util/thread.h File be/src/util/thread.h: http://gerrit.cloudera.org:8080/#/c/9053/2/be/src/util/thread.h@191 PS2, Line 191: const ThreadDebugInfo* parent_thread_info This should go to the front (behind category or functor) now since it's strictly an input parameter and we order them as inputs, then output. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 01 Feb 2018 22:54:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 1: (6 comments) Thanks for the comments. I checked the callsites of Thread::Create() and the name is always set explicitly. If it weren't, then Thread::SuperviseThread() would create a unique thread name with the system thread id. Therefore, as far as parent threads outlive their child threads we can restore the full thread trees. I think this "2 gen by Instance Worker Foo" might be more relevant for the general purpose thread pools where we just submit work items and don't set any names at all. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h@70 PS1, Line 70: void ExtractInfoFromParent(const ThreadDebugInfo* parent) { > Have you considered storing the parent thread ID (possibly both system and Yeah it's a good idea. I created a nested struct called ParentTdi for the parent fields, to keep things organized. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h@71 PS1, Line 71: DCHECK(parent != nullptr); > Is there a benefit of not allowing nullptr vs. allowing and ignoring it? Th No, I think we can allow and ignore nullptrs. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/exec/hdfs-scan-node.cc@a353 PS1, Line 353: > My understanding is that this is no longer needed because the information i Yes. I checked all these places with GDB to test if instance id propagates to the child TDI. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h File be/src/util/thread.h: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h@183 PS1, Line 183: static void SuperviseThread(const string& name, const string& category, > Our convention is usually to keep the std:: namespace prefix in header file Oops, I think I've copied the new signature from the .cc I got wonder how did it compile, then I realized that I've included common/names.h in thread-debug-info.h. Removed the include. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h@185 PS1, Line 185: parent_thread_info > The comment should explain what this parameter does (and whether it can be Added comment, and also made the parameter a pointer to const. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.cc File be/src/util/thread.cc: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.cc@350 PS1, Line 350: if (parent_thread_info) thread_debug_info.ExtractInfoFromParent(parent_thread_info); > We usually make the comparison to != nullptr explicit to make it more clear Removed the check since ExtractInfoFromParent() now accepts nullptrs as well. And since it is an inline function we don't save the cost of a function call with this check. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 01 Feb 2018 13:28:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Hello Lars Volker, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9053 to look at the new patch set (#2). Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id and also saves its parent's name. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record their parent's name, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 99 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/9053/2 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9053 ) Change subject: IMPALA-6416: extend Thread::Create to track instance id .. Patch Set 1: (6 comments) I left some inline comments. I feel it would be better to find a way to propagate thread names indefinitely, compared to propagating them for one generation (which is my understanding of this patch). Currently it allows us to remove the call to the explicit setter in the scan node, but one generation further the same mechanism will not work anymore. Maybe we could think of deriving names automatically (if they are not explicitly set), e.g. by prepending the parent name with an increasing counter. If "Instance Worker Foo" creates a child, then its name could be "1 gen by Instance Worker Foo", and if this child spawns a thread w/out an explicit name, it could be called "2 gen by Instance Worker Foo" or similar. This way we could programatically capture full thread trees. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h@70 PS1, Line 70: void ExtractInfoFromParent(const ThreadDebugInfo* parent) { Have you considered storing the parent thread ID (possibly both system and boost) in the ThreadDebugInfo, too? While every thread had its own Info it seemed superflous to store it again, but for the parent threads it might be helpful, especially when thinking of programmatic ways to analyze the thread graph. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/common/thread-debug-info.h@71 PS1, Line 71: DCHECK(parent != nullptr); Is there a benefit of not allowing nullptr vs. allowing and ignoring it? The latter could make the calling code a bit more concise, but I may be missing something. If you keep the requirement that parent != nullptr, you could consider making this a static factory that creates and pre-populates a TDI. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/exec/hdfs-scan-node.cc@a353 PS1, Line 353: My understanding is that this is no longer needed because the information is now kept in the parent TDI. Is this correct? http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h File be/src/util/thread.h: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h@183 PS1, Line 183: static void SuperviseThread(const string& name, const string& category, Our convention is usually to keep the std:: namespace prefix in header files but drop it in .cc files. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.h@185 PS1, Line 185: parent_thread_info The comment should explain what this parameter does (and whether it can be null), especially since we usually pass input parameters as const& and output parameters as non-const ptrs. http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.cc File be/src/util/thread.cc: http://gerrit.cloudera.org:8080/#/c/9053/1/be/src/util/thread.cc@350 PS1, Line 350: if (parent_thread_info) thread_debug_info.ExtractInfoFromParent(parent_thread_info); We usually make the comparison to != nullptr explicit to make it more clear what is happening. -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 25 Jan 2018 00:24:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6416: extend Thread::Create to track instance id
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9053 Change subject: IMPALA-6416: extend Thread::Create to track instance id .. IMPALA-6416: extend Thread::Create to track instance id This commit builds upon IMPALA-3703. Each thread that was created through Thread::Create() has a ThreadDebugInfo object on the stack frame of Thread::SuperviseThread(). This object has stack allocated char buffers that can be read during a debug session even if we only have minidumps. However, with the old solution ThreadDebugInfo::instance_id was set manually for each thread. It is too easy to forget to set instance_id every time we create a new thread. This commit has the assumption that if a thread has an instance id associated, then the threads spawned by it will always work on the same instance id. In Thread::StartThread the parent thread passes its ThreadDebugInfo object to its child who copies the instance id and also saves its parent's name. This means if we set ThreadDebugInfo::instance_id in some "root thread", then all descendant threads will annotate themselves with the instance id automatically. Since threads also record their parent's name, it might be also possible to reconstruct the thread creation graph. With GDB I tested if it copies the instance id at every place where we previously needed to set it manually. I added an automated test to thread-debug-info-test.cc Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 --- M be/src/common/thread-debug-info-test.cc M be/src/common/thread-debug-info.cc M be/src/common/thread-debug-info.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/thread.cc M be/src/util/thread.h 8 files changed, 65 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/9053/1 -- To view, visit http://gerrit.cloudera.org:8080/9053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I27de2962cf0b224c17b685d77dcba3bf2e9db187 Gerrit-Change-Number: 9053 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy