Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1044413002:
Record async "task backtraces" (Closed)
DescriptionRecord async "task backtraces"
Places a small array in PendingTask that records the past 4 PostTasks
that result in this task being run. In essence, it creates a backtrace
of the asynchronous call stack resulting in this task being run.
This array is copied onto the stack during execution so that it is
available in crash dumps.
BUG=690197
Review-Url: https://codereview.chromium.org/1044413002
Cr-Commit-Position: refs/heads/master@{#449471}
Committed: https://chromium.googlesource.com/chromium/src/+/4f13f74b94d6f0a39b73a832217c5ccc2f02e60f
Patch Set 1 #
Total comments: 1
Patch Set 2 : reviving from the dead #Patch Set 3 : moar 00 like. #
Total comments: 6
Patch Set 4 : Now with unittests! #
Total comments: 31
Patch Set 5 : Don't duplicate the posted_from in stacktrace. #
Total comments: 3
Patch Set 6 : Fix off-by-one. #Patch Set 7 : STL ALL THE THINGS #
Total comments: 4
Patch Set 8 : c++11 is hard; let's go shopping. #
Total comments: 2
Patch Set 9 : Fix moar comments. #Patch Set 10 : All ur base R useless to us. #Patch Set 11 : All ur base R useless to us. #Patch Set 12 : Actually use thread_b when I said I would. #
Total comments: 4
Patch Set 13 : Make the test data plus comments match up. #
Total comments: 2
Patch Set 14 : traces R hard. #
Total comments: 2
Patch Set 15 : More wording fixes. #
Messages
Total messages: 72 (36 generated)
ajwong@chromium.org changed reviewers: + danakj@chromium.org, jamesr@chromium.org, thakis@chromium.org
Crazy? Interesting?
On 2015/03/31 22:40:50, awong (On leave) wrote: > Crazy? Interesting? Mentioned on IM, but crash dumps are (apparently) getting discarded now post-processing in most cases, so this information would generally not be available for the majority of crashes unless also integrated into the processor or UI (or crash reverses their policy - and perhaps they already have in the past few days)
On 2015/04/01 02:09:48, Ryan Sleevi wrote: > On 2015/03/31 22:40:50, awong (On leave) wrote: > > Crazy? Interesting? > > Mentioned on IM, but crash dumps are (apparently) getting discarded now > post-processing in most cases, so this information would generally not be > available for the majority of crashes unless also integrated into the processor > or UI (or crash reverses their policy - and perhaps they already have in the > past few days) I think the goal here is that if we have this info available that we get it surfaced in the UI. Would that be something possible?
On 2015/04/03 16:46:28, danakj wrote: > On 2015/04/01 02:09:48, Ryan Sleevi wrote: > > On 2015/03/31 22:40:50, awong (On leave) wrote: > > > Crazy? Interesting? > > > > Mentioned on IM, but crash dumps are (apparently) getting discarded now > > post-processing in most cases, so this information would generally not be > > available for the majority of crashes unless also integrated into the > processor > > or UI (or crash reverses their policy - and perhaps they already have in the > > past few days) > > I think the goal here is that if we have this info available that we get it > surfaced in the UI. Would that be something possible? Totally possible. I've talked to crash team in the past about it. It's mostly a question of whether or not someone writes the chromium side code and then follows up with them. I think this could be *extremely* helpful in a number of debugging situations. However, I also stop working on this official in 3 hours so I won't have time to pursue it...
https://codereview.chromium.org/1044413002/diff/1/base/threading/worker_pool_... File base/threading/worker_pool_posix.cc (right): https://codereview.chromium.org/1044413002/diff/1/base/threading/worker_pool_... base/threading/worker_pool_posix.cc:144: MessageLoop::current()->current_pending_task_, The use of MessageLoop::current()->current_pending_task_ worries me. I think maybe we need to ask for this off the MessageLoop's task_runner() instead. But I'm not totally sure.. It's easy to get lost in MessageLoop. Ryan do you have a good position to comment on this being right/wrong? I can dig a whole bunch more if you don't have some guidance here :)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ajwong@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: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by ajwong@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 2015/04/03 17:18:35, danakj (slow) wrote: > https://codereview.chromium.org/1044413002/diff/1/base/threading/worker_pool_... > File base/threading/worker_pool_posix.cc (right): > > https://codereview.chromium.org/1044413002/diff/1/base/threading/worker_pool_... > base/threading/worker_pool_posix.cc:144: > MessageLoop::current()->current_pending_task_, > The use of MessageLoop::current()->current_pending_task_ worries me. I think > maybe we need to ask for this off the MessageLoop's task_runner() instead. But > I'm not totally sure.. > > It's easy to get lost in MessageLoop. Ryan do you have a good position to > comment on this being right/wrong? > > I can dig a whole bunch more if you don't have some guidance here :) @danakj: Ping! Wanna take another stab? I'm writing unittests as we speak, but wanted to vet the overall approach first.
Description was changed from ========== Proof of concept task backtrace code. Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it's create a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=none ========== to ========== Proof of concept task backtrace code. Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=none ==========
So high level what about other ways to run tasks. https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?... https://cs.chromium.org/chromium/src/base/threading/worker_pool_posix.cc?rcl=... https://cs.chromium.org/chromium/src/base/task_scheduler/task_tracker.cc?rcl=... I'm likely overlooking some. But I think there's no MessageLoop in worker pools for instance, or on TaskScheduler threads. https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... base/message_loop/message_loop.cc:324: current_pending_task_(NULL), nullptr https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... base/message_loop/message_loop.h:455: // Be very careful when using this variable. careful in what way, what are the risks? https://codereview.chromium.org/1044413002/diff/60001/base/threading/worker_p... File base/threading/worker_pool_posix.cc (right): https://codereview.chromium.org/1044413002/diff/60001/base/threading/worker_p... base/threading/worker_pool_posix.cc:15: #include "base/message_loop/message_loop.h" looks out of place
Added a unittest. Regarding how non-messageloop ways of running tests.. Urgh. I think the right thing there is to probably create a ThreadLocalStorage for current_pending_task and move that out of MessageLoop. However, I don't understand all these types of execution contexts well enough at this point. How'd you feel about getting this in for MessageLoops first and opening a bug to migrate to other types of systems? I think this is still useful for MessageLoop as is... https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... base/message_loop/message_loop.cc:324: current_pending_task_(NULL), On 2017/02/07 23:16:46, danakj (slow) wrote: > nullptr Done. https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... base/message_loop/message_loop.h:455: // Be very careful when using this variable. On 2017/02/07 23:16:46, danakj (slow) wrote: > careful in what way, what are the risks? rewrote comment. https://codereview.chromium.org/1044413002/diff/60001/base/threading/worker_p... File base/threading/worker_pool_posix.cc (right): https://codereview.chromium.org/1044413002/diff/60001/base/threading/worker_p... base/threading/worker_pool_posix.cc:15: #include "base/message_loop/message_loop.h" On 2017/02/07 23:16:46, danakj (slow) wrote: > looks out of place oops. old code. deleted.
The CQ bit was checked by ajwong@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...
Regarding the threads w/o a MessageLoop that run Tasks, it looks like SequencedWorkerPool and WorkerPool are the two fundamental objects. The TaskScheduler seems to dispatch to these two WorkerPools. Within each of these ThreaMain/ThreadLoop functions, we need to find a way to create the "current_pending_task" concept... This probably means reviving the API where PendingTask has |parent_task| in the constructor. Then we outsource the tracking of the current task to the user of PendingTask. As is, all that's going to happen is the breadcrumb will truncate when we hit a WorkerPool which is no worse than the current state. I'd like to shift this into a different CL since it's medium surgery and this review has taken a long time as is ;) On 2017/02/08 01:55:50, awong wrote: > Added a unittest. > > Regarding how non-messageloop ways of running tests.. Urgh. > > I think the right thing there is to probably create a ThreadLocalStorage for > current_pending_task and move that out of MessageLoop. > > However, I don't understand all these types of execution contexts well enough at > this point. > > How'd you feel about getting this in for MessageLoops first and opening a bug to > migrate to other types of systems? I think this is still useful for MessageLoop > as is... > > https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... > File base/message_loop/message_loop.cc (right): > > https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... > base/message_loop/message_loop.cc:324: current_pending_task_(NULL), > On 2017/02/07 23:16:46, danakj (slow) wrote: > > nullptr > > Done. > > https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... > File base/message_loop/message_loop.h (right): > > https://codereview.chromium.org/1044413002/diff/60001/base/message_loop/messa... > base/message_loop/message_loop.h:455: // Be very careful when using this > variable. > On 2017/02/07 23:16:46, danakj (slow) wrote: > > careful in what way, what are the risks? > > rewrote comment. > > https://codereview.chromium.org/1044413002/diff/60001/base/threading/worker_p... > File base/threading/worker_pool_posix.cc (right): > > https://codereview.chromium.org/1044413002/diff/60001/base/threading/worker_p... > base/threading/worker_pool_posix.cc:15: #include > "base/message_loop/message_loop.h" > On 2017/02/07 23:16:46, danakj (slow) wrote: > > looks out of place > > oops. old code. deleted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
danakj@chromium.org changed reviewers: - jamesr@chromium.org
On Tue, Feb 7, 2017 at 9:17 PM, <ajwong@chromium.org> wrote: > Regarding the threads w/o a MessageLoop that run Tasks, it looks like > SequencedWorkerPool and WorkerPool are the two fundamental objects. The > TaskScheduler seems to dispatch to these two WorkerPools. > So, TaskScheduler is a replacement for these pools. Currently you make SequencedWorkerPool defer to TaskScheduler at runtime for experimenting. Eventually it should supercede them. > Within each of these ThreaMain/ThreadLoop functions, we need to find a way > to > create the "current_pending_task" concept... This probably means reviving > the > API where PendingTask has |parent_task| in the constructor. > That seems good, I prefer that to looking up global state in the constructor. Then we outsource the tracking of the current task to the user of > PendingTask. > +1 As is, all that's going to happen is the breadcrumb will truncate when we > hit a > WorkerPool which is no worse than the current state. > OK let me fully understand what magic is going on so I can confirm this. :) > I'd like to shift this into a different CL since it's medium surgery and > this > review has taken a long time as is ;) > What are the risks in this CL that we should be worried about. Would you say there's any risk of performance costs or something else? > > > > > On 2017/02/08 01:55:50, awong wrote: > > Added a unittest. > > > > Regarding how non-messageloop ways of running tests.. Urgh. > > > > I think the right thing there is to probably create a ThreadLocalStorage > for > > current_pending_task and move that out of MessageLoop. > > > > However, I don't understand all these types of execution contexts well > enough > at > > this point. > > > > How'd you feel about getting this in for MessageLoops first and opening > a bug > to > > migrate to other types of systems? I think this is still useful for > MessageLoop > > as is... > > > > > https://codereview.chromium.org/1044413002/diff/60001/ > base/message_loop/message_loop.cc > > File base/message_loop/message_loop.cc (right): > > > > > https://codereview.chromium.org/1044413002/diff/60001/ > base/message_loop/message_loop.cc#newcode324 > > base/message_loop/message_loop.cc:324: current_pending_task_(NULL), > > On 2017/02/07 23:16:46, danakj (slow) wrote: > > > nullptr > > > > Done. > > > > > https://codereview.chromium.org/1044413002/diff/60001/ > base/message_loop/message_loop.h > > File base/message_loop/message_loop.h (right): > > > > > https://codereview.chromium.org/1044413002/diff/60001/ > base/message_loop/message_loop.h#newcode455 > > base/message_loop/message_loop.h:455: // Be very careful when using this > > variable. > > On 2017/02/07 23:16:46, danakj (slow) wrote: > > > careful in what way, what are the risks? > > > > rewrote comment. > > > > > https://codereview.chromium.org/1044413002/diff/60001/ > base/threading/worker_pool_posix.cc > > File base/threading/worker_pool_posix.cc (right): > > > > > https://codereview.chromium.org/1044413002/diff/60001/ > base/threading/worker_pool_posix.cc#newcode15 > > base/threading/worker_pool_posix.cc:15: #include > > "base/message_loop/message_loop.h" > > On 2017/02/07 23:16:46, danakj (slow) wrote: > > > looks out of place > > > > oops. old code. deleted. > > > > https://codereview.chromium.org/1044413002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Since this is > 1 patch (and should anyway in case of followup issues) can you file a bug for this and add to the CL
On 2015/04/01 02:09:48, Ryan Sleevi wrote: > On 2015/03/31 22:40:50, awong (On leave) wrote: > > Crazy? Interesting? > > Mentioned on IM, but crash dumps are (apparently) getting discarded now > post-processing in most cases, so this information would generally not be > available for the majority of crashes unless also integrated into the processor > or UI (or crash reverses their policy - and perhaps they already have in the > past few days) TBH I'm not sure what this means - crash/ certainly shows stack traces, does crashpad not acquire them in a way that would see this change? Is this relevant even still?
On 2017/02/08 16:56:50, danakj (slow) wrote: > TBH I'm not sure what this means - crash/ certainly shows stack traces, does > crashpad not acquire them in a way that would see this change? Is this relevant > even still? Still relevant. The use of base::Debug::Alias() only manifests the contents of that memory with the raw memorydump. The crash serving infrastructure is more aggressively discarding those memory dump files after it's processed and generated the stack traces and relevant data, but it doesn't process/extract this information from the task in order to generate a more comprehensive backtrace (as that'd require more processing and examination during the stackwalk code to use the IP stored in the variable and the stack data we're including, which right now we don't) Put differently, if you have a .dmp file (in the case of Windows), you can totally use this information to walk it back. If you have a .core file (on Linux/macOS), you can totally use this information to walk it back. The problem is that most crash reports, after generating the processed data for the interface, were (and AIUI, are) discarding the .dmp file, and we don't submit the .core file, so if you're simply browsing the crash interface, you won't necessarily be able to walk this data - unless it's also integrated into the .dmp/crashpad processor to populate the UI and database.
On Wed, Feb 8, 2017 at 12:00 PM, <rsleevi@chromium.org> wrote: > On 2017/02/08 16:56:50, danakj (slow) wrote: > > TBH I'm not sure what this means - crash/ certainly shows stack traces, > does > > crashpad not acquire them in a way that would see this change? Is this > relevant > > even still? > > Still relevant. > > The use of base::Debug::Alias() only manifests the contents of that memory > with > the raw memorydump. The crash serving infrastructure is more aggressively > discarding those memory dump files after it's processed and generated the > stack > traces and relevant data, but it doesn't process/extract this information > from > the task in order to generate a more comprehensive backtrace (as that'd > require > more processing and examination during the stackwalk code to use the IP > stored > in the variable and the stack data we're including, which right now we > don't) > Oh I had not yet figured out what magic debug::Alias does to change the call stack. It's literally an empty function XD Put differently, if you have a .dmp file (in the case of Windows), you can > totally use this information to walk it back. If you have a .core file (on > Linux/macOS), you can totally use this information to walk it back. The > problem > is that most crash reports, after generating the processed data for the > interface, were (and AIUI, are) discarding the .dmp file, and we don't > submit > the .core file, so if you're simply browsing the crash interface, you won't > necessarily be able to walk this data - unless it's also integrated into > the > .dmp/crashpad processor to populate the UI and database. > This makes sense, thanks. Does crashpad make any use of the current |program_counter| either? A git grep doesn't find that name but maybe I'm doing it wrong. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1044413002/diff/80001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/debug/task_annotat... base/debug/task_annotator.cc:49: const void* task_backtrace[4]; can you static assert that this is the same size as the pending task's array? (or 1 larger if we dedupe the 0th element) https://codereview.chromium.org/1044413002/diff/80001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/message_loop/messa... base/message_loop/message_loop.cc:429: current_pending_task_ = NULL; nullptr https://codereview.chromium.org/1044413002/diff/80001/base/pending_task.cc File base/pending_task.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/pending_task.cc#ne... base/pending_task.cc:31: memcpy(&task_backtrace[1], &parent_task->task_backtrace[0], I can't help but notice that we store the program_location twice now, once in posted_from and once in task_backtrace. If you made the task_backtrace only 3 large, and copied the parent_task's program_counter to 0 instead of this task's, then we wouldn't have that redundancy. TaskAnnotator makes its own copy anyways also so it's not requiring this info to be a contiguous block. wdyt? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:17: can you wrap everything in a nested anon namespace so we don't have possibility to collide with other tests or anything? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:20: PendingTaskTest() {} = default https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:22: ~PendingTaskTest() override {} = default https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:25: typedef std::vector<const void*> ExpectedTrace; using instead of typedef https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:27: static void VerifyTraceAndPost(const scoped_refptr<TaskRunner>& task_runner, just TaskRunner*, this doesn't need to use scoped_refptr abilities https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:29: const std::vector<const void*>& expected_trace, you could take this by value, it's always an rvalue https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:65: ExpectedTrace({location4.program_counter(), location3.program_counter(), you could just pass the init list directly, why the cast to vector here? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:67: base::Bind(&DoNothing)); don't need base:: inside base https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:95: base::MessageLoop loop; // Implicitly "thread a" comment needs . https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:128: thread_c.message_loop()->task_runner(), location_b1, prefer thread_c.task_runner() over thread_c.message_loop()->task_runner() https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:135: thread_c.message_loop()->task_runner(), location_b1, should this be thread_b? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:141: // Push one frame onto the stack in thread a then pass to thread b. This puts 2 frames on thread a, right? Cuz the first block also says push one frame but it posts to a new thread right away. Here it self-posts first then bounces. https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:144: thread_c.message_loop()->task_runner(), location_b0, the comment says b but this variable says c?
On 2017/02/08 17:36:00, danakj (slow) wrote: > This makes sense, thanks. Does crashpad make any use of the current > |program_counter| either? A git grep doesn't find that name but maybe I'm > doing it wrong. No. The current |program_counter| is useful for active debugging, however, because you can easily see that in a debugger watchlist and how it works. To be clear: I'm totally supportive of this overall direction, and it will directly make debugging complex call chains easier. However, it won't necessarily propagate through the crash system, so the Aliasing aspect probably isn't necessary (Alias exists to try to ensure it doesn't get optimized out and gets included in the crashdumps' grabbing of the stack space) If we do want this information in crash/, that'll require more changes. But again, that doesn't block this CL, just manages expectations :)
Description was changed from ========== Proof of concept task backtrace code. Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=none ========== to ========== Proof of concept task backtrace code. Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=690197 ==========
ajwong@chromium.org changed reviewers: - thakis@chromium.org
ajwong@chromium.org changed required reviewers: + danakj@chromium.org
PTAL So, IIRC, breakpad (and I assume crashpad) snags the stack of the running threads. The Alias() thing was put in with program_counter way back to ensure the program counter made it into the stack. Alias() itself is an empty function that just ensures that the optimizer can't infer through the function boundary to realize the symbol isn't actually used. The other half of this work is in crash, yes. I don't expect to end right here. :) Regarding performance hit, this should be basically an assignment followed by a continuous 4-word copy per PostTask, and in each runloop. I expect it to be completely negligible. Especially if TaskScheduler is already doing crazy like looking things up by traits per post...I think this is far into the noise. On 2017/02/08 18:22:05, Ryan Sleevi wrote: > On 2017/02/08 17:36:00, danakj (slow) wrote: > > This makes sense, thanks. Does crashpad make any use of the current > > |program_counter| either? A git grep doesn't find that name but maybe I'm > > doing it wrong. > > No. The current |program_counter| is useful for active debugging, however, > because you can easily see that in a debugger watchlist and how it works. > > To be clear: I'm totally supportive of this overall direction, and it will > directly make debugging complex call chains easier. However, it won't > necessarily propagate through the crash system, so the Aliasing aspect probably > isn't necessary (Alias exists to try to ensure it doesn't get optimized out and > gets included in the crashdumps' grabbing of the stack space) > > If we do want this information in crash/, that'll require more changes. But > again, that doesn't block this CL, just manages expectations :)
The CQ bit was checked by ajwong@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...
https://codereview.chromium.org/1044413002/diff/80001/base/debug/task_annotat... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/debug/task_annotat... base/debug/task_annotator.cc:49: const void* task_backtrace[4]; On 2017/02/08 18:06:22, danakj (slow) wrote: > can you static assert that this is the same size as the pending task's array? > (or 1 larger if we dedupe the 0th element) good idea. done. Actually did better and just tied the size of the array in the stack to the array in the struct. https://codereview.chromium.org/1044413002/diff/80001/base/message_loop/messa... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/message_loop/messa... base/message_loop/message_loop.cc:429: current_pending_task_ = NULL; On 2017/02/08 18:06:22, danakj (slow) wrote: > nullptr Done. https://codereview.chromium.org/1044413002/diff/80001/base/pending_task.cc File base/pending_task.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/pending_task.cc#ne... base/pending_task.cc:31: memcpy(&task_backtrace[1], &parent_task->task_backtrace[0], On 2017/02/08 18:06:22, danakj (slow) wrote: > I can't help but notice that we store the program_location twice now, once in > posted_from and once in task_backtrace. If you made the task_backtrace only 3 > large, and copied the parent_task's program_counter to 0 instead of this task's, > then we wouldn't have that redundancy. TaskAnnotator makes its own copy anyways > also so it's not requiring this info to be a contiguous block. wdyt? sure... nice catch! I actually left the backtrace at size 4 so now we have 5 frames. :)
Cool, there's a bunch of test comments that I assume you're looking thru, but the non-test code LG.
https://codereview.chromium.org/1044413002/diff/100001/base/debug/task_annota... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/100001/base/debug/task_annota... base/debug/task_annotator.cc:52: sizeof(task_backtrace)); The compiler disagrees with me, you want size of pending_task's here, or this - 1.
https://codereview.chromium.org/1044413002/diff/100001/base/debug/task_annota... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/100001/base/debug/task_annota... base/debug/task_annotator.cc:52: sizeof(task_backtrace)); On 2017/02/08 23:40:28, danakj (slow) wrote: > The compiler disagrees with me, you want size of pending_task's here, or this - > 1. Possibly consider using std::array and STL things to avoid such problems.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by ajwong@chromium.org to run a CQ dry run
https://codereview.chromium.org/1044413002/diff/100001/base/debug/task_annota... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/100001/base/debug/task_annota... base/debug/task_annotator.cc:52: sizeof(task_backtrace)); On 2017/02/08 23:41:04, danakj (slow) wrote: > On 2017/02/08 23:40:28, danakj (slow) wrote: > > The compiler disagrees with me, you want size of pending_task's here, or this > - > > 1. > > Possibly consider using std::array and STL things to avoid such problems. EEk. Good catch. STLed everything.
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.
What did you think of these comments? I think maybe they were missed? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... https://codereview.chromium.org/1044413002/diff/110001/base/debug/task_annota... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/110001/base/debug/task_annota... base/debug/task_annotator.cc:52: std::tuple_size<decltype(pending_task->task_backtrace)>::value + 1> can you pull the std::tuple_size<decltype(pending_task->task_backtrace)>::value part out into a constexpr variable, it would make this easier to read. i wonder if we should make a base::array_size http://stackoverflow.com/a/16866128 https://codereview.chromium.org/1044413002/diff/110001/base/pending_task.cc File base/pending_task.cc (right): https://codereview.chromium.org/1044413002/diff/110001/base/pending_task.cc#n... base/pending_task.cc:32: parent_task->task_backtrace.begin() + (task_backtrace.size() - 1), this is end() - 1 right?
https://codereview.chromium.org/1044413002/diff/110001/base/debug/task_annota... File base/debug/task_annotator.cc (right): https://codereview.chromium.org/1044413002/diff/110001/base/debug/task_annota... base/debug/task_annotator.cc:52: std::tuple_size<decltype(pending_task->task_backtrace)>::value + 1> On 2017/02/09 16:39:50, danakj (slow) wrote: > can you pull the std::tuple_size<decltype(pending_task->task_backtrace)>::value > part out into a constexpr variable, it would make this easier to read. done. > i wonder if we should make a base::array_size > http://stackoverflow.com/a/16866128 Honestly seems overkill unless this sort of dependent array size becomes more prevalent. I am a conservative when it comes to adding concepts into low level libraries. https://codereview.chromium.org/1044413002/diff/110001/base/pending_task.cc File base/pending_task.cc (right): https://codereview.chromium.org/1044413002/diff/110001/base/pending_task.cc#n... base/pending_task.cc:32: parent_task->task_backtrace.begin() + (task_backtrace.size() - 1), On 2017/02/09 16:39:51, danakj (slow) wrote: > this is end() - 1 right? Yes...initially I thought it was illegal to subtract of end()...but apparently it's okay for random access iterators. (verified in [random.access.iterator]).
The CQ bit was checked by ajwong@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...
https://codereview.chromium.org/1044413002/diff/130001/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/130001/base/pending_task_unit... base/pending_task_unittest.cc:38: ->current_pending_task_->task_backtrace.size(), nit: this would probably wrap a lot nicer if u put the current_pending_task_ into a temp var, and bonus points cuz u access it again below so it can be reused
https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:17: On 2017/02/08 18:06:23, danakj (slow) wrote: > can you wrap everything in a nested anon namespace so we don't have possibility > to collide with other tests or anything? can't since we're friended by MessageLoop. Added note to bug to remove friending and add anon namespace back when done. https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:20: PendingTaskTest() {} On 2017/02/08 18:06:23, danakj (slow) wrote: > = default Done. https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:22: ~PendingTaskTest() override {} On 2017/02/08 18:06:23, danakj (slow) wrote: > = default Done. https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:25: typedef std::vector<const void*> ExpectedTrace; On 2017/02/08 18:06:23, danakj (slow) wrote: > using instead of typedef done. I don't recognize this language anymore. :-/ https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:27: static void VerifyTraceAndPost(const scoped_refptr<TaskRunner>& task_runner, On 2017/02/08 18:06:23, danakj (slow) wrote: > just TaskRunner*, this doesn't need to use scoped_refptr abilities So I sorta in general prefer to use const scoped_refptr<>& in these cases. The capture in Bind() already stores a refcounted version. Moving to a rawpointer involves another cast, and an Unretained(). I don't think it makes sense... https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:29: const std::vector<const void*>& expected_trace, On 2017/02/08 18:06:22, danakj (slow) wrote: > you could take this by value, it's always an rvalue Is there a benefit to this? It seems like it's just introducing more of a chance for a copy...especially through a bind? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:65: ExpectedTrace({location4.program_counter(), location3.program_counter(), On 2017/02/08 18:06:23, danakj (slow) wrote: > you could just pass the init list directly, why the cast to vector here? nope! base::Bind() doesn't understand initializer lists! :-/ https://codereview.chromium.org/1044413002/diff/130001/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/130001/base/pending_task_unit... base/pending_task_unittest.cc:38: ->current_pending_task_->task_backtrace.size(), On 2017/02/09 20:34:42, danakj (slow) wrote: > nit: this would probably wrap a lot nicer if u put the current_pending_task_ > into a temp var, and bonus points cuz u access it again below so it can be > reused Done.
The CQ bit was checked by ajwong@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...
https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:27: static void VerifyTraceAndPost(const scoped_refptr<TaskRunner>& task_runner, On 2017/02/09 20:56:05, awong wrote: > On 2017/02/08 18:06:23, danakj (slow) wrote: > > just TaskRunner*, this doesn't need to use scoped_refptr abilities > > So I sorta in general prefer to use const scoped_refptr<>& in these cases. The > capture in Bind() already stores a refcounted version. Moving to a rawpointer > involves another cast, and an Unretained(). I don't think it makes sense... In that case I agree, but I think it would just require a .get() on the bind. Unretained is only required for calling member methods, I don't *think* it applies to static? https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:29: const std::vector<const void*>& expected_trace, On 2017/02/09 20:56:05, awong wrote: > On 2017/02/08 18:06:22, danakj (slow) wrote: > > you could take this by value, it's always an rvalue > > Is there a benefit to this? It seems like it's just introducing more of a chance > for a copy...especially through a bind? Oh that's a good point about Bind, I was reading it like a straight function call, it would copy it out of Callback until this is OnceCallback. I was just thinking it's less text to parse. https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:65: ExpectedTrace({location4.program_counter(), location3.program_counter(), On 2017/02/09 20:56:05, awong wrote: > On 2017/02/08 18:06:23, danakj (slow) wrote: > > you could just pass the init list directly, why the cast to vector here? > > nope! base::Bind() doesn't understand initializer lists! :-/ Ah! Right, me forgetting the Bind again here.
https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:27: static void VerifyTraceAndPost(const scoped_refptr<TaskRunner>& task_runner, On 2017/02/09 21:04:14, danakj (slow) wrote: > On 2017/02/09 20:56:05, awong wrote: > > On 2017/02/08 18:06:23, danakj (slow) wrote: > > > just TaskRunner*, this doesn't need to use scoped_refptr abilities > > > > So I sorta in general prefer to use const scoped_refptr<>& in these cases. The > > capture in Bind() already stores a refcounted version. Moving to a rawpointer > > involves another cast, and an Unretained(). I don't think it makes sense... > > In that case I agree, but I think it would just require a .get() on the bind. > Unretained is only required for calling member methods, I don't *think* it > applies to static? For class methods, there is a special casing of argument one to disallow raw pointers. However, for other arguments, there is a a check to ensure we don't accidentally store RefCounted types without a smart pointer in the BindStorage (this is NOT the same as the callee function signature). https://cs.chromium.org/chromium/src/base/bind_internal.h?l=495 Thus if we kept a raw pointer in the callee signature, then either we need to unwrap the stored arugment from a smart pointer (which doesn't work) or we need to tell Bind() to store a raw pointer (which is what the Unretained() hint does). https://codereview.chromium.org/1044413002/diff/80001/base/pending_task_unitt... base/pending_task_unittest.cc:67: base::Bind(&DoNothing)); On 2017/02/08 18:06:23, danakj (slow) wrote: > don't need base:: inside base Done.
Thanks! LGTM and I think the test is testing the code but it's a bit inconsistent, comments below: https://codereview.chromium.org/1044413002/diff/210001/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/210001/base/pending_task_unit... base/pending_task_unittest.cc:132: thread_c.message_loop()->task_runner(), location_b0, location_b1, The names of the locations are a bit inconsistent. Like here it's going to thread_c but the next location is location_b1 (named for where its coming from). for task_a1 its going to thread_b, and the next location is location_b0 (named for where its going) https://codereview.chromium.org/1044413002/diff/210001/base/pending_task_unit... base/pending_task_unittest.cc:143: // Push one frame onto the stack in thread a then pass to thread b. The comment explanations are helpful but also a bit inconsistent. Here it says: - push a stack in thread a => posts a task on thread_a. - pass to thread b => posts a task on thread_b For the block above it says: - push a frame on the stack => i don't see this in the code - then spawn two tasks one in thread b => posts to thread_b - and one in thread c => posts to thread_c For the topmost block it says: - push one frame on the stack in thread c => I don't see this it posts to thread_a? - then pass to thread a => it posts a 2nd task onto thread a We should make the code and comments consistent here at least.
Description was changed from ========== Proof of concept task backtrace code. Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=690197 ========== to ========== Record async "task backtraces" Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=690197 ==========
https://codereview.chromium.org/1044413002/diff/210001/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/210001/base/pending_task_unit... base/pending_task_unittest.cc:132: thread_c.message_loop()->task_runner(), location_b0, location_b1, On 2017/02/09 21:16:27, danakj (slow) wrote: > The names of the locations are a bit inconsistent. Like here it's going to > thread_c but the next location is location_b1 (named for where its coming from). > > for task_a1 its going to thread_b, and the next location is location_b0 (named > for where its going) That's actually me doing the backtrace wrong. :-/ Fixed to set the "next_from_here" to c0 since it's the first frame on thread c. https://codereview.chromium.org/1044413002/diff/210001/base/pending_task_unit... base/pending_task_unittest.cc:143: // Push one frame onto the stack in thread a then pass to thread b. On 2017/02/09 21:16:27, danakj (slow) wrote: > The comment explanations are helpful but also a bit inconsistent. > > Here it says: > - push a stack in thread a => posts a task on thread_a. > - pass to thread b => posts a task on thread_b > > For the block above it says: > - push a frame on the stack => i don't see this in the code > - then spawn two tasks one in thread b => posts to thread_b > - and one in thread c => posts to thread_c > > For the topmost block it says: > - push one frame on the stack in thread c => I don't see this it posts to > thread_a? > - then pass to thread a => it posts a 2nd task onto thread a > > We should make the code and comments consistent here at least. I think the comments were actually right and the code was wrong. I matched up the code to the comments.
https://codereview.chromium.org/1044413002/diff/120009/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/120009/base/pending_task_unit... base/pending_task_unittest.cc:123: loop.task_runner(), location_c0, location_c1, This should be thread_c.task_runner() I think? https://codereview.chromium.org/1044413002/diff/120009/base/pending_task_unit... base/pending_task_unittest.cc:130: // thread b and one in thread c using location_b1. Thanks! just drop the "using location_b1" here, and the "Push one frame onto the stack in thread b then" cuz this is just spawning 2 tasks?
PTAL
Hehe million back and forths. It LGTM. Last one just the comment https://codereview.chromium.org/1044413002/diff/240001/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/240001/base/pending_task_unit... base/pending_task_unittest.cc:130: // Push one frame onto the stack in thread b then spawn two tasks, one in This line's still misplaced? As we don't do anything before spawning the 2 tasks. It's kinda talking about the RunTwo post in the block below
committing. https://codereview.chromium.org/1044413002/diff/240001/base/pending_task_unit... File base/pending_task_unittest.cc (right): https://codereview.chromium.org/1044413002/diff/240001/base/pending_task_unit... base/pending_task_unittest.cc:130: // Push one frame onto the stack in thread b then spawn two tasks, one in On 2017/02/09 21:43:41, danakj (slow) wrote: > This line's still misplaced? As we don't do anything before spawning the 2 > tasks. It's kinda talking about the RunTwo post in the block below blargh. Okay, I switched the blocks to describe what they are dong on their target thread which I think is what it was effectively becoming and is clearer.
The CQ bit was checked by ajwong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1044413002/#ps260001 (title: "More wording fixes.")
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": 260001, "attempt_start_ts": 1486677228732950,
"parent_rev": "47ee91d0dc7bfc1afed32d6ce1d7c5311859d1d2", "commit_rev":
"4f13f74b94d6f0a39b73a832217c5ccc2f02e60f"}
Message was sent while issue was closed.
Description was changed from ========== Record async "task backtraces" Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=690197 ========== to ========== Record async "task backtraces" Places a small array in PendingTask that records the past 4 PostTasks that result in this task being run. In essence, it creates a backtrace of the asynchronous call stack resulting in this task being run. This array is copied onto the stack during execution so that it is available in crash dumps. BUG=690197 Review-Url: https://codereview.chromium.org/1044413002 Cr-Commit-Position: refs/heads/master@{#449471} Committed: https://chromium.googlesource.com/chromium/src/+/4f13f74b94d6f0a39b73a832217c... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:260001) as https://chromium.googlesource.com/chromium/src/+/4f13f74b94d6f0a39b73a832217c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
