Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007343003:
Move stack trace extraction code out of TickSample::Init (Closed)
DescriptionMove stack trace extraction code out of TickSample::Init
Make it a part of V8 API GetStackSample function.
Also expose external_callback_entry in SampleInfo to break dependency
of clients on internal V8 structures.
BUG=v8:4789
Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd
Committed: https://crrev.com/2f863593d11c9459bd6e4eb721f39fffaa4ccd58
Cr-Original-Commit-Position: refs/heads/master@{#36831}
Cr-Commit-Position: refs/heads/master@{#36836}
Patch Set 1 #
Total comments: 10
Patch Set 2 : make MSAN happy #
Messages
Total messages: 30 (10 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/patch-status/2007343003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007343003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
alph@chromium.org changed reviewers: + jochen@chromium.org, lpy@chromium.org, yangguo@chromium.org
Yang, could you please take a look.
Thanks for doing this, I left a few comments. https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:622: if (!pc_entry && !sample.has_external_callback) { I am not sure here, my question is what the relationship between `has_external_callback` and the type of top stack frame is? https://codereview.chromium.org/2007343003/diff/1/src/profiler/tick-sample.cc File src/profiler/tick-sample.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/tick-sample.cc... src/profiler/tick-sample.cc:168: return true; If I understand correctly, when there's no sample collected, TickSample::Init will set pc = 0 and also a timestamp, so shouldn't this returns false when frames_count is 0?
https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:622: if (!pc_entry && !sample.has_external_callback) { On 2016/05/26 16:19:16, lpy wrote: > I am not sure here, my question is what the relationship between > `has_external_callback` and the type of top stack frame is? If we're in an external callback we don't want to dig much into what in the top frame. I also didn't want to alter the current semantics much with this patch, so I left external_callback_entry and tos as a union. https://codereview.chromium.org/2007343003/diff/1/src/profiler/tick-sample.cc File src/profiler/tick-sample.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/tick-sample.cc... src/profiler/tick-sample.cc:168: return true; On 2016/05/26 16:19:16, lpy wrote: > If I understand correctly, when there's no sample collected, TickSample::Init > will set pc = 0 and also a timestamp, so shouldn't this returns false when > frames_count is 0? Zero frames sample is a legit sample. The function returns false when it fails to collect a sample. there's currently just a single case -- when the sample falls into a no-frame region.
lgtm from me side with a few comments. https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:622: if (!pc_entry && !sample.has_external_callback) { On 2016/05/26 16:34:13, alph wrote: > On 2016/05/26 16:19:16, lpy wrote: > > I am not sure here, my question is what the relationship between > > `has_external_callback` and the type of top stack frame is? > > If we're in an external callback we don't want to dig much into what in the top > frame. I also didn't want to alter the current semantics much with this patch, > so I left external_callback_entry and tos as a union. I think the comment above should be updated. https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:647: if (!sample.has_external_callback) { maybe the comment above should be updated.
yang@, could you please take a look?
https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:622: if (!pc_entry && !sample.has_external_callback) { This definitely does something different than the old code. We could be not in an external callback, but also not in one of the frame types. For example we could be in a stub frame, or a C entry frame. Is the code map ready for this? Can you add some explanation/comment here? https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:647: if (!sample.has_external_callback) { On 2016/05/26 20:49:17, lpy wrote: > maybe the comment above should be updated. Is the comment above no longer valid?
https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:647: if (!sample.has_external_callback) { On 2016/05/30 10:54:24, Yang wrote: > On 2016/05/26 20:49:17, lpy wrote: > > maybe the comment above should be updated. > > Is the comment above no longer valid? The comment is still legit.
ptal https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2007343003/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:622: if (!pc_entry && !sample.has_external_callback) { On 2016/05/30 10:54:24, Yang wrote: > This definitely does something different than the old code. We could be not in > an external callback, but also not in one of the frame types. For example we > could be in a stub frame, or a C entry frame. Is the code map ready for this? > Can you add some explanation/comment here? Sorry, I should have explained it in more details. Here it is. Since it cannot find the code entry for PC, that means the current function is a native one. As long as it is not an external callback it could be e.g. a V8 runtime/helper. We don't really know which one, so we cannot do much here. However, what we can do is to make sure that if the top function happens to be a frameless invocation, we try to find out it's caller, which is hopefully pointed by the top of SP. I could expose the top_frame_type to V8 API and keep the code the same, but I didn't want to trash the API without a strong reason. The code is indeed changed a bit, but I can't think of any externally visible effect of this change. If you have an idea of test case that would be affected by the change, please let me know.
friendly ping
On 2016/06/07 15:56:41, alph wrote: > friendly ping lgtm.
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007343003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 ========== to ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831}
Message was sent while issue was closed.
Description was changed from ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831} ========== to ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831} ==========
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2049903002/ by alph@chromium.org. The reason for reverting is: Make MSAN arm bot flaky.
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpy@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2007343003/#ps20001 (title: "make MSAN happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007343003/20001
Message was sent while issue was closed.
Description was changed from ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831} ========== to ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831} ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Cr-Commit-Position: refs/heads/master@{#36831} ========== to ========== Move stack trace extraction code out of TickSample::Init Make it a part of V8 API GetStackSample function. Also expose external_callback_entry in SampleInfo to break dependency of clients on internal V8 structures. BUG=v8:4789 Committed: https://crrev.com/70acfe39c07322144f5fe9b40bb584a8b1099ffd Committed: https://crrev.com/2f863593d11c9459bd6e4eb721f39fffaa4ccd58 Cr-Original-Commit-Position: refs/heads/master@{#36831} Cr-Commit-Position: refs/heads/master@{#36836} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2f863593d11c9459bd6e4eb721f39fffaa4ccd58 Cr-Commit-Position: refs/heads/master@{#36836} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
