Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483473002:
Include calling address in tracked activities. (Closed)
|
Created:
4 years, 1 month ago by bcwhite Modified:
4 years, 1 month ago Reviewers:
manzagop (departed) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude calling address in tracked activities.
BUG=620813
Committed: https://crrev.com/21f6c57d7b9d19e479f1d1bdf2b423da65129f16
Cr-Commit-Position: refs/heads/master@{#431262}
Patch Set 1 #
Total comments: 8
Patch Set 2 : force inlining of methods that call GetProgramCounter() #
Messages
Total messages: 22 (12 generated)
The CQ bit was checked by bcwhite@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...
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
BTW... I looked at extracting the Task callback address and using that as the PC instead of the PC where the ScopedTaskRunActivity is instantiated. I don't think that's feasible. Bind (near as I can figure) somehow creates inline wrappers to call the eventual final function -- these wrappers handle the myriad of parameters being passed from the method creating the callback. Thus, there's no practical way I can see to unwind this to know the final destination. Even if Callback somehow kept this, it could still be wrong because callbacks can wrap other callbacks. But I'm open to ideas!
lgtm! Thanks for doing this. Yeah, I wonder how we could do the task's address. It looks like a callback has a BindState which has a polymorphic_invoke field which is the function? I wonder if we could add an accessor to it, based on how Run() does it. https://cs.chromium.org/chromium/src/base/callback.h?sq=package:chromium&dr=C... https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyze... base/debug/activity_analyzer.h:134: ProgramLocation GetProgramLocationFromAddress(uint64_t address); I don't think we need this on the client. Did you have a specific use in mind? https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:353: PushActivity(::tracked_objects::GetProgramCounter(), origin, type, data); Comment that we'd like this function to be inlined to get a useful program counter? Adding the inline keyword may help give the compiler a stronger hint.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I actually spent a couple hours getting the polymorphic_invoke value out but it turned out to be the address of a a dynamically created "wrapper" method. It might be possible to add code to that object that would return the actual function being called but the code that defines Callback was too complicated to figure out in the time I had. Also, it wouldn't be guaranteed correct because you can Bind to an existing Callback to create a new callback that requires fewer parameters. Whatever code extracts the destination address would either fail in that case or require some added complexity. https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyze... base/debug/activity_analyzer.h:134: ProgramLocation GetProgramLocationFromAddress(uint64_t address); On 2016/11/09 16:48:44, manzagop wrote: > I don't think we need this on the client. Did you have a specific use in mind? I figured that your uploader would want this -- that it would convert every address while filling the protobuf. No? https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:353: PushActivity(::tracked_objects::GetProgramCounter(), origin, type, data); On 2016/11/09 16:48:44, manzagop wrote: > Comment that we'd like this function to be inlined to get a useful program > counter? > Adding the inline keyword may help give the compiler a stronger hint. Good idea. There's an ALWAYS_INLINE option. I'll add that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyze... base/debug/activity_analyzer.h:134: ProgramLocation GetProgramLocationFromAddress(uint64_t address); On 2016/11/09 19:41:39, bcwhite wrote: > On 2016/11/09 16:48:44, manzagop wrote: > > I don't think we need this on the client. Did you have a specific use in mind? > > I figured that your uploader would want this -- that it would convert every > address while filling the protobuf. > No? I'm thinking we can do that server side, since we already have the machinery to symbolize. We'll only need to add some crumbs about modules offset / sizes for modules of interest (chrome). https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:353: PushActivity(::tracked_objects::GetProgramCounter(), origin, type, data); On 2016/11/09 19:41:39, bcwhite wrote: > On 2016/11/09 16:48:44, manzagop wrote: > > Comment that we'd like this function to be inlined to get a useful program > > counter? > > Adding the inline keyword may help give the compiler a stronger hint. > > Good idea. There's an ALWAYS_INLINE option. I'll add that. Didn't know about that! Our use is sparse enough that we shouldn't impact code size.
On 2016/11/09 19:41:39, bcwhite wrote: > I actually spent a couple hours getting the polymorphic_invoke value out but it > turned out to be the address of a a dynamically created "wrapper" method. > > It might be possible to add code to that object that would return the actual > function being called but the code that defines Callback was too complicated to > figure out in the time I had. > > Also, it wouldn't be guaranteed correct because you can Bind to an existing > Callback to create a new callback that requires fewer parameters. Whatever code > extracts the destination address would either fail in that case or require some > added complexity. Thanks. Sorry, I hadn't connected the code I was seeing to what you'd already said. :) > https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyzer.h > File base/debug/activity_analyzer.h (right): > > https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyze... > base/debug/activity_analyzer.h:134: ProgramLocation > GetProgramLocationFromAddress(uint64_t address); > On 2016/11/09 16:48:44, manzagop wrote: > > I don't think we need this on the client. Did you have a specific use in mind? > > I figured that your uploader would want this -- that it would convert every > address while filling the protobuf. > No? > > https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker.h > File base/debug/activity_tracker.h (right): > > https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker... > base/debug/activity_tracker.h:353: > PushActivity(::tracked_objects::GetProgramCounter(), origin, type, data); > On 2016/11/09 16:48:44, manzagop wrote: > > Comment that we'd like this function to be inlined to get a useful program > > counter? > > Adding the inline keyword may help give the compiler a stronger hint. > > Good idea. There's an ALWAYS_INLINE option. I'll add that.
https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyzer.h File base/debug/activity_analyzer.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_analyze... base/debug/activity_analyzer.h:134: ProgramLocation GetProgramLocationFromAddress(uint64_t address); On 2016/11/10 15:01:35, manzagop wrote: > On 2016/11/09 19:41:39, bcwhite wrote: > > On 2016/11/09 16:48:44, manzagop wrote: > > > I don't think we need this on the client. Did you have a specific use in > mind? > > > > I figured that your uploader would want this -- that it would convert every > > address while filling the protobuf. > > No? > > I'm thinking we can do that server side, since we already have the machinery to > symbolize. We'll only need to add some crumbs about modules offset / sizes for > modules of interest (chrome). I'm going to add the module "crumbs" support into the tracker and have it provide an API to extract them. Since I'll have that, I can easily include this method as well; perhaps it will be useful for real-time analysis if that ever gets implemented. The StackSamplingProfiler converts everything to module+offset when uploading its data so it's probably a good idea to to do it the same way here. If that works for you, then you can start using this API right now and just handle the 0,0 case appropriately.. https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker.h File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2483473002/diff/1/base/debug/activity_tracker... base/debug/activity_tracker.h:353: PushActivity(::tracked_objects::GetProgramCounter(), origin, type, data); On 2016/11/10 15:01:36, manzagop wrote: > On 2016/11/09 19:41:39, bcwhite wrote: > > On 2016/11/09 16:48:44, manzagop wrote: > > > Comment that we'd like this function to be inlined to get a useful program > > > counter? > > > Adding the inline keyword may help give the compiler a stronger hint. > > > > Good idea. There's an ALWAYS_INLINE option. I'll add that. > > Didn't know about that! Our use is sparse enough that we shouldn't impact code > size. Heh! I thought I'd seen it in the past and a search showed it conveniently in base... put there exactly 24 hours before! I had to "git pull" to use it. :-)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Include calling address in tracked activities. BUG=620813 ========== to ========== Include calling address in tracked activities. BUG=620813 Committed: https://crrev.com/21f6c57d7b9d19e479f1d1bdf2b423da65129f16 Cr-Commit-Position: refs/heads/master@{#431262} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/21f6c57d7b9d19e479f1d1bdf2b423da65129f16 Cr-Commit-Position: refs/heads/master@{#431262} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
