Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2461003002:
[profiler] Emit runtime call stats into sampling profile (Closed)
Description[profiler] Emit runtime call stats into sampling profile
These are added to the sampler stack trace when RCS are
enabled.
Resource name for a RCS frame is reported as "V8Runtime".
Counter names match ones from src/counters.h
BUG=chromium:660428
Committed: https://crrev.com/aee3542fcf3b10e950a8d7cb2aaff4927a66ed7c
Cr-Commit-Position: refs/heads/master@{#40658}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 26 (9 generated)
The CQ bit was checked by alph@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...
alph@chromium.org changed reviewers: + cbruni@chromium.org, lpy@chromium.org, yangguo@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:766: entry = new CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, I don't think they should be considered as CodeEntry's instead I think we should build a new Node type for them And we should inject them statically, since the counter names are predefined. I do not think they should live on the same stack either.
On 2016/10/28 19:51:32, fmeawad wrote: > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > File src/profiler/profile-generator.cc (right): > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > src/profiler/profile-generator.cc:766: entry = new > CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, > I don't think they should be considered as CodeEntry's instead I think we should > build a new Node type for them > > And we should inject them statically, since the counter names are predefined. > > I do not think they should live on the same stack either. lpy pointed out that we cannot have same created statically, since we create different leafs based on the location in the stack, so you can ignore that part. But I don't think we should either reuse CodeEntry or the same stack.
On 2016/10/28 19:55:47, fmeawad wrote: > On 2016/10/28 19:51:32, fmeawad wrote: > > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > > File src/profiler/profile-generator.cc (right): > > > > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > > src/profiler/profile-generator.cc:766: entry = new > > CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, > > I don't think they should be considered as CodeEntry's instead I think we > should > > build a new Node type for them > > > > And we should inject them statically, since the counter names are predefined. > > > > I do not think they should live on the same stack either. > > lpy pointed out that we cannot have same created statically, since we create > different leafs based on the location in the stack, so you can ignore that part. > But I don't think we should either reuse CodeEntry or the same stack. I didn't get this part. Counters are static per isolate and there's a 1:1 for Counter to CodeEntry. So I think I can create them statically.
https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:766: entry = new CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, On 2016/10/28 19:51:32, fmeawad wrote: > I don't think they should be considered as CodeEntry's instead I think we should > build a new Node type for them I don't think we need an extra class here. CodeEntry is an universal one. We use tags to distinguish between different frame types. I will probably introduce a tag for it in an upcoming patch. > And we should inject them statically, since the counter names are predefined. I was going to do that, but postponed for later. It's just more code, but should speedup the lookup. > I do not think they should live on the same stack either. Having them on the same stack is a big advantage as otherwise it would be impossible to tell how these interleave with user functions. Having them on the same stack simplifies data structure and also gives us flexibility to split them into two stacks on post processing if needed.
On 2016/10/28 20:13:35, alph wrote: > On 2016/10/28 19:55:47, fmeawad wrote: > > On 2016/10/28 19:51:32, fmeawad wrote: > > > > > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > > > File src/profiler/profile-generator.cc (right): > > > > > > > > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > > > src/profiler/profile-generator.cc:766: entry = new > > > CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, > > > I don't think they should be considered as CodeEntry's instead I think we > > should > > > build a new Node type for them > > > > > > And we should inject them statically, since the counter names are > predefined. > > > > > > I do not think they should live on the same stack either. > > > > lpy pointed out that we cannot have same created statically, since we create > > different leafs based on the location in the stack, so you can ignore that > part. > > But I don't think we should either reuse CodeEntry or the same stack. > > I didn't get this part. > Counters are static per isolate and there's a 1:1 for Counter to CodeEntry. > So I think I can create them statically. Then I must have confused a CodeEntry with a leaf id, I assume that every different stack will have a different leaf id
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/28 20:15:34, alph wrote: > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > File src/profiler/profile-generator.cc (right): > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > src/profiler/profile-generator.cc:766: entry = new > CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, > On 2016/10/28 19:51:32, fmeawad wrote: > > I don't think they should be considered as CodeEntry's instead I think we > should > > build a new Node type for them > > I don't think we need an extra class here. CodeEntry is an universal one. We use > tags to distinguish between different frame types. I will probably introduce a > tag for it in an upcoming patch. > > > And we should inject them statically, since the counter names are predefined. > > I was going to do that, but postponed for later. It's just more code, but should > speedup the lookup. > > > I do not think they should live on the same stack either. > > Having them on the same stack is a big advantage as otherwise it would be > impossible to tell how these interleave with user functions. Having them on the > same stack simplifies data structure and also gives us flexibility to split them > into two stacks on post processing if needed. Why not utilize the ability of the new sampling format to maintain multiple stacks per sample?
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
On 2016/10/28 20:17:05, fmeawad wrote: > On 2016/10/28 20:15:34, alph wrote: > > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > > File src/profiler/profile-generator.cc (right): > > > > > https://codereview.chromium.org/2461003002/diff/1/src/profiler/profile-genera... > > src/profiler/profile-generator.cc:766: entry = new > > CodeEntry(CodeEventListener::FUNCTION_TAG, counter->name, > > On 2016/10/28 19:51:32, fmeawad wrote: > > > I don't think they should be considered as CodeEntry's instead I think we > > should > > > build a new Node type for them > > > > I don't think we need an extra class here. CodeEntry is an universal one. We > use > > tags to distinguish between different frame types. I will probably introduce a > > tag for it in an upcoming patch. > > > > > And we should inject them statically, since the counter names are > predefined. > > > > I was going to do that, but postponed for later. It's just more code, but > should > > speedup the lookup. > > > > > I do not think they should live on the same stack either. > > > > Having them on the same stack is a big advantage as otherwise it would be > > impossible to tell how these interleave with user functions. Having them on > the > > same stack simplifies data structure and also gives us flexibility to split > them > > into two stacks on post processing if needed. > > Why not utilize the ability of the new sampling format to maintain multiple > stacks per sample? I don't see a point to separate these stacks. They exist on the same stack and we want to show them (at least in timeline) on the same stack. It would be virtually impossible to merge two stacks back together, but splitting is easy. Plus the arguments I listed above.
Message was sent while issue was closed.
CodeEntry can be created statically. What I mean is that ProfileNode can't be created statically, since the parent of a runtime counter is not always static.
Message was sent while issue was closed.
On 2016/10/28 20:22:51, lpy wrote: > CodeEntry can be created statically. What I mean is that ProfileNode can't be > created statically, since the parent of a runtime counter is not always static. That's true, but we were talking about CodeEntry'es, not ProfileNode's.
Message was sent while issue was closed.
On 2016/10/28 20:22:51, lpy wrote: > CodeEntry can be created statically. What I mean is that ProfileNode can't be > created statically, since the parent of a runtime counter is not always static. Merging them should not be hard if they are using the same chunk/timedelta for both of them. I am questioning the decision whether they should be on the same stack or not: Same Stack argument: this information describes the state of the running function itself, and in another sense, the runtime stats stack is the C++ code running on top of the javascript stack to execute it Different Stack argument: This information has a different type and different granularity. Runtime Stats are not for say a pseudo stack frames, but more of a system state, they do not represent code in the same way CodeEntries does. Also, we want to make Runtime Stats a first class citizen in Chrome tracing, but putting them on the cpuprofilestack, it sets us back a bit.
Message was sent while issue was closed.
verwaest@chromium.org changed reviewers: + verwaest@chromium.org
Message was sent while issue was closed.
Even though I've only seen the screenshot, I think the merged stack could provide additional insights that we haven't looked at yet. On the other hand I hope it's not designed as an absolute replacement indeed; it would be very useful to have our standard way of using it as first-class citizen.
Message was sent while issue was closed.
On 2016/10/28 20:31:24, fmeawad wrote: > On 2016/10/28 20:22:51, lpy wrote: > > CodeEntry can be created statically. What I mean is that ProfileNode can't be > > created statically, since the parent of a runtime counter is not always > static. > > Merging them should not be hard if they are using the same chunk/timedelta for > both of them. > > I am questioning the decision whether they should be on the same stack or not: > Same Stack argument: this information describes the state of the running > function itself, and in another sense, the runtime stats stack is the C++ code > running on top of the javascript stack to execute it > Different Stack argument: This information has a different type and different > granularity. Runtime Stats are not for say a pseudo stack frames, but more of a > system state, they do not represent code in the same way CodeEntries does. > Also, we want to make Runtime Stats a first class citizen in Chrome tracing, but > putting them on the cpuprofilestack, it sets us back a bit. Sorry, I don't see how would you merge two stacks, as the interleaving information is lost. We definitely want to have RCS integrated into JS samples on timeline, as it gives us new insights on the code execution. I'm not saying this should be the only way we get RCS out of V8. We can also emit it in a separate stack independently (e.g. under a different tracing category) if there's a need.
Message was sent while issue was closed.
Description was changed from ========== [profiler] Emit runtime call stats into sampling profile These are added to the sampler stack trace when RCS are enabled. Resource name for a RCS frame is reported as "V8Runtime". Counter names match ones from src/counters.h BUG=chromium:660428 ========== to ========== [profiler] Emit runtime call stats into sampling profile These are added to the sampler stack trace when RCS are enabled. Resource name for a RCS frame is reported as "V8Runtime". Counter names match ones from src/counters.h BUG=chromium:660428 Committed: https://crrev.com/aee3542fcf3b10e950a8d7cb2aaff4927a66ed7c Cr-Commit-Position: refs/heads/master@{#40658} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/aee3542fcf3b10e950a8d7cb2aaff4927a66ed7c Cr-Commit-Position: refs/heads/master@{#40658} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
