Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1221873002:
Change sampler code events phase from instant to meta. (Closed)
|
Created:
5 years, 5 months ago by alph Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, erikwright+watch_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, pfeldman, tracing+reviews_chromium.org, wfh+watch_chromium.org, yurys Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange sampler code events phase from instant to meta.
This is according to the specification.
BUG=406277
Committed: https://crrev.com/bc1343daeb77b33031de79f2c104f1efaf560c99
Cr-Commit-Position: refs/heads/master@{#377708}
Patch Set 1 #
Total comments: 2
Patch Set 2 : removed scope #
Total comments: 4
Patch Set 3 : addressing comment. #Patch Set 4 : rebaseline + make use of AddMetadataEvent function #
Total comments: 4
Patch Set 5 : drop legacy functions #Patch Set 6 : Fix a comment #
Messages
Total messages: 46 (13 generated)
alph@chromium.org changed reviewers: + dsinclair@chromium.org, yurys@chromium.org
dsinclair@chromium.org changed reviewers: + nduca@chromium.org
The one concern I have is, internal metadata events won't get dropped because of the way we add them to the buffer. These events will get dropped when the buffer loops around. Are we concerned with losing them? https://codereview.chromium.org/1221873002/diff/1/content/renderer/devtools/v... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/1221873002/diff/1/content/renderer/devtools/v... content/renderer/devtools/v8_sampling_profiler.cc:331: "JitCodeAdded", TRACE_EVENT_SCOPE_THREAD, "data", What does SCOPE_THREAD mean for a metadata event?
The patch doesn't affect the lifetime of the code events. The sampling cannot afford loosing them, that's why sampling is currently disabled for cycle-buffer mode. There was an idea to keep meta events in a separate buffer that won't drop them during the recording. https://codereview.chromium.org/1221873002/diff/1/content/renderer/devtools/v... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/1221873002/diff/1/content/renderer/devtools/v... content/renderer/devtools/v8_sampling_profiler.cc:331: "JitCodeAdded", TRACE_EVENT_SCOPE_THREAD, "data", On 2015/06/30 18:14:17, dsinclair wrote: > What does SCOPE_THREAD mean for a metadata event? Not really needed, but I thought it might be useful to specify the event is related to a particular thread. Removed.
Here's a link to the Doc for caseq@ https://docs.google.com/document/d/1DDS8UJySywkYZDpmgocGBAmB7jVZJyMvN22f_6wxq...
caseq@chromium.org changed reviewers: + caseq@chromium.org
From what I understood from offline discussion with alph, the plan is to use a separate buffer for metadata events that won't be circular even in the continuous tracing mode. This looks weird to me because we won't be able support "continuous tracing" contract given that there's no upper bound for the number JitCodeAdded / JitCodeMoved events -- so we would eventually need to either discard them or die with OOM. So why not just give up on supporting JS profiling with continuous mode?
On 2015/07/01 13:17:33, caseq wrote: > From what I understood from offline discussion with alph, the plan is to use a > separate buffer for metadata events that won't be circular even in the > continuous tracing mode. This looks weird to me because we won't be able support > "continuous tracing" contract given that there's no upper bound for the number > JitCodeAdded / JitCodeMoved events -- so we would eventually need to either > discard them or die with OOM. So why not just give up on supporting JS profiling > with continuous mode? We do want to see the stacks in continuous mode. Users don't usually know which mode they're in and it would be confusing if the stacks didn't show up for them. We need to figure out some way to support it sanely I think.
On 2015/07/01 13:20:37, dsinclair wrote: > On 2015/07/01 13:17:33, caseq wrote: > > From what I understood from offline discussion with alph, the plan is to use a > > separate buffer for metadata events that won't be circular even in the > > continuous tracing mode. This looks weird to me because we won't be able > support > > "continuous tracing" contract given that there's no upper bound for the number > > JitCodeAdded / JitCodeMoved events -- so we would eventually need to either > > discard them or die with OOM. So why not just give up on supporting JS > profiling > > with continuous mode? > > > We do want to see the stacks in continuous mode. Users don't usually know which > mode they're in and it would be confusing if the stacks didn't show up for them. > We need to figure out some way to support it sanely I think. Well, with current approach we won't be able to make it really continuous due to unbound growth of code map meta events, will we? One approach that I see would be to reverse the code map management logic, i.e. dump the code map snapshot at the end of the trace recording rather than the start of the trace and reconstruct code map at other moments of time by going backwards through JitCodeMoved events & co. This would, however, require replacing JitCodeAdded with JitCodeRemoved events, presumable emitted during the GC. I've no idea whether it's actually feasible, though.
On 2015/07/01 13:35:30, caseq wrote: > On 2015/07/01 13:20:37, dsinclair wrote: > > On 2015/07/01 13:17:33, caseq wrote: > > > From what I understood from offline discussion with alph, the plan is to use > a > > > separate buffer for metadata events that won't be circular even in the > > > continuous tracing mode. This looks weird to me because we won't be able > > support > > > "continuous tracing" contract given that there's no upper bound for the > number > > > JitCodeAdded / JitCodeMoved events -- so we would eventually need to either > > > discard them or die with OOM. So why not just give up on supporting JS > > profiling > > > with continuous mode? > > > > > > We do want to see the stacks in continuous mode. Users don't usually know > which > > mode they're in and it would be confusing if the stacks didn't show up for > them. > > We need to figure out some way to support it sanely I think. > > Well, with current approach we won't be able to make it really continuous due to > unbound growth of code map meta events, will we? One approach that I see would > be to reverse the code map management logic, i.e. dump the code map snapshot at > the end of the trace recording rather than the start of the trace and > reconstruct code map at other moments of time by going backwards through > JitCodeMoved events & co. This would, however, require replacing JitCodeAdded > with JitCodeRemoved events, presumable emitted during the GC. I've no idea > whether it's actually feasible, though. alph@ would that work? Seems like it would get around the losing events issue as we'd always have the full map at the end?
On 2015/07/01 13:47:21, dsinclair wrote: > On 2015/07/01 13:35:30, caseq wrote: > > On 2015/07/01 13:20:37, dsinclair wrote: > > > On 2015/07/01 13:17:33, caseq wrote: > > > > From what I understood from offline discussion with alph, the plan is to > use > > a > > > > separate buffer for metadata events that won't be circular even in the > > > > continuous tracing mode. This looks weird to me because we won't be able > > > support > > > > "continuous tracing" contract given that there's no upper bound for the > > number > > > > JitCodeAdded / JitCodeMoved events -- so we would eventually need to > either > > > > discard them or die with OOM. So why not just give up on supporting JS > > > profiling > > > > with continuous mode? > > > > > > > > > We do want to see the stacks in continuous mode. Users don't usually know > > which > > > mode they're in and it would be confusing if the stacks didn't show up for > > them. > > > We need to figure out some way to support it sanely I think. > > > > Well, with current approach we won't be able to make it really continuous due > to > > unbound growth of code map meta events, will we? One approach that I see would > > be to reverse the code map management logic, i.e. dump the code map snapshot > at > > the end of the trace recording rather than the start of the trace and > > reconstruct code map at other moments of time by going backwards through > > JitCodeMoved events & co. This would, however, require replacing JitCodeAdded > > with JitCodeRemoved events, presumable emitted during the GC. I've no idea > > whether it's actually feasible, though. > > > alph@ would that work? Seems like it would get around the losing events issue as > we'd always have the full map at the end? V8 does not emit JitCodeRemoved events, because GC does not really know which code objects getting deleted (you know it marks the live objects and discards the rest). We could implement injection of CodeRemoved events, but that would require maintaining the code map on the backend side. However that contradicts with the approach we've chosen (Variant A) -- no additional logic or event processing in the backend. Btw, maintaining the code map would be just cheating as it theoretically has no upper bound in memory grows. So it wouldn't be a pure cyclic buffer anyway. I'd vote for just dropping support for cyclic-buffer mode.
> We could implement injection of CodeRemoved events, but that would require > maintaining the code map on the backend side. > However that contradicts with the approach we've chosen (Variant A) -- no > additional logic or event processing in the backend. > Btw, maintaining the code map would be just cheating as it theoretically has no > upper bound in memory grows. So it wouldn't be a pure cyclic buffer anyway. Well, as long as we process hypothetical CodeRemoved events the code map size is bound by current JS code size, so if CodeRemoved proves to be feasible, we're fine.
On 2015/07/01 15:02:24, caseq wrote: > > We could implement injection of CodeRemoved events, but that would require > > maintaining the code map on the backend side. > > However that contradicts with the approach we've chosen (Variant A) -- no > > additional logic or event processing in the backend. > > Btw, maintaining the code map would be just cheating as it theoretically has > no > > upper bound in memory grows. So it wouldn't be a pure cyclic buffer anyway. > > Well, as long as we process hypothetical CodeRemoved events the code map size is > bound by current JS code size, so if CodeRemoved proves to be feasible, we're > fine. No, to generate CodeRemoved events we need to maintain the code map and it can grow. Let me give an example. The code map may have entries for objects located in v8 heap space at address 0x100000, then GC kicks in and the whole heap is moved to address 0x200000 with some objects moved and some deleted, but we can't know these objects are deleted, so we have to keep them in the map forever.
https://codereview.chromium.org/1221873002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1221873002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:558: // TRACE_EVENT_METADATA* events are injected by the sampling profiler. nit: remove the bit about being injected by the profile and say something like. 'events are information related to other injected events, not events in their own right'. https://codereview.chromium.org/1221873002/diff/20001/content/renderer/devtoo... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/1221873002/diff/20001/content/renderer/devtoo... content/renderer/devtools/v8_sampling_profiler.cc:343: case v8::JitCodeEvent::CODE_REMOVED: So, this isn't usually emitted, but it can be?
https://codereview.chromium.org/1221873002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1221873002/diff/20001/base/trace_event/trace_... base/trace_event/trace_event.h:558: // TRACE_EVENT_METADATA* events are injected by the sampling profiler. On 2015/07/02 14:17:05, dsinclair wrote: > nit: remove the bit about being injected by the profile and say something like. > 'events are information related to other injected events, not events in their > own right'. Done. Thanks. https://codereview.chromium.org/1221873002/diff/20001/content/renderer/devtoo... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/1221873002/diff/20001/content/renderer/devtoo... content/renderer/devtools/v8_sampling_profiler.cc:343: case v8::JitCodeEvent::CODE_REMOVED: Looks like this part is not implemented. https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/heap/mark-c...
lgtm. I think this is good to land for now, but I think we need to make sure we make this work in continuous trace mode in the future. Otherwise, we're going to get a lot of traces recorded that are missing stacks and users won't understand what's happening. (trace-viewer remembers your buffer setting, so if you pick continuous you'll be in continuous until you change it again, and people forget what mode they're in when recording)
lgtm. I think this is good to land for now, but I think we need to make sure we make this work in continuous trace mode in the future. Otherwise, we're going to get a lot of traces recorded that are missing stacks and users won't understand what's happening. (trace-viewer remembers your buffer setting, so if you pick continuous you'll be in continuous until you change it again, and people forget what mode they're in when recording)
On 2015/07/02 15:04:23, dsinclair wrote: > I think this is good to land for now, but I think we need to make sure we > make this work in continuous trace mode in the future. So does this make sense to land without actually having any support for retaining these meta-events at the moment? The initial approach does not seem to conform to continuous tracing contract and the one we lately discussed does not require changing event phases to meta.
On 2015/07/02 15:09:38, caseq wrote: > On 2015/07/02 15:04:23, dsinclair wrote: > > I think this is good to land for now, but I think we need to make sure we > > make this work in continuous trace mode in the future. > > So does this make sense to land without actually having any support for > retaining these meta-events at the moment? The initial approach does not seem to > conform to continuous tracing contract and the one we lately discussed does not > require changing event phases to meta. I believe it was mentioned that sampling is currently disabled in continuous mode? So, I think landing this is fine as we won't lose any of the metadata events. I just think we need to make sure we figure out how to make it work in continuous mode. I think in all cases, these aren't 'real' events in and of themselves, they provide extra information for other events? If that's correct then I think metadata is the right event type for them.
Was the plan to still land this?
Description was changed from ========== Change sampler code events phase from instant to meta. This is according to the specification. BUG=406277 ========== to ========== Change sampler code events phase from instant to meta. This is according to the specification. BUG=406277 ==========
dsinclair@chromium.org changed reviewers: - dsinclair@chromium.org
On 2015/08/05 18:13:31, dsinclair wrote: > Was the plan to still land this? I'm up for that. Will update the CL. I just noticed there is an AddMetadataEvent method that adds the meta event into a dedicated buffer. However it lacks the category field, so add the meta events added via this API is kinda unconditional. I'm going to add the category field to it if there're no objections.
alph@chromium.org changed reviewers: + fmeawad@chromium.org
ptal
https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:188: // on the convertable value will be called at flush time. After a second thought I think it is better to eliminate the version with no category. https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:1344: TimeTicks(), ThreadTicks(), TRACE_EVENT_PHASE_METADATA, I think you should call the other method here to reduce code duplication.
https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:188: // on the convertable value will be called at flush time. On 2016/02/16 21:47:37, fmeawad wrote: > After a second thought I think it is better to eliminate the version with no > category. ok. done https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1221873002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:1344: TimeTicks(), ThreadTicks(), TRACE_EVENT_PHASE_METADATA, On 2016/02/16 21:47:37, fmeawad wrote: > I think you should call the other method here to reduce code duplication. They were quite distinct... But anyway one of them is now gone.
non-owner lgtm.
Dan, do you want to look at it?
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
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/1221873002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1221873002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alph@chromium.org changed reviewers: + pfeldman@chromium.org
Pavel, could you please do the owner review.
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/1221873002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1221873002/100001
Message was sent while issue was closed.
Description was changed from ========== Change sampler code events phase from instant to meta. This is according to the specification. BUG=406277 ========== to ========== Change sampler code events phase from instant to meta. This is according to the specification. BUG=406277 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Change sampler code events phase from instant to meta. This is according to the specification. BUG=406277 ========== to ========== Change sampler code events phase from instant to meta. This is according to the specification. BUG=406277 Committed: https://crrev.com/bc1343daeb77b33031de79f2c104f1efaf560c99 Cr-Commit-Position: refs/heads/master@{#377708} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bc1343daeb77b33031de79f2c104f1efaf560c99 Cr-Commit-Position: refs/heads/master@{#377708} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
