Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2323483005:
[tracing] Add filtering mode in TraceLog (Closed)
Description[tracing] Add filtering mode in TraceLog
This CL does:
1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and
disabled with FILTERING_MODE independently from RECORDING MODE. The
TraceLog::mode_ is a bitmap.
2. In FILTERING_MODE trace buffer is not created and trace events are
passed to filters but not recorded.
3. The filters can be added only by FILTERING_MODE and trace config.
Once enabled, the filters cannot be changed until the filters are
disabled.
4. HeapProfilerFilter is enabled using FILTERING_MODE when
"--enable-heap-profiling" flag is passed, so that pseudo stack is
recorded for all allocations, irrespective of tracing.
BUG=625170
Committed: https://crrev.com/20d2eeed2c735fbf018b9a9dffa810d51a25940d
Cr-Commit-Position: refs/heads/master@{#425156}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes. #
Total comments: 1
Patch Set 3 : Fixes. #
Total comments: 20
Patch Set 4 : SetEnabled takes a bitmap. #
Total comments: 37
Patch Set 5 : Primiano's comments. #
Total comments: 16
Patch Set 6 : Nit. #
Total comments: 4
Patch Set 7 : Add a comment, and renames. #
Dependent Patchsets: Messages
Total messages: 72 (50 generated)
ssid@chromium.org changed reviewers: + oysteine@chromium.org, primiano@chromium.org
Look at this after https://codereview.chromium.org/2316403002/. I would like to do this change. But, I understand that this breaks some assumptions. Any suggestion on how to achieve the filtering enabled always?
Description was changed from ========== [tracing] "--enable-heap-profiling" will keep event filtering always turned on The Heap profiler has to capture trace events always when "--enable-heap-profiling" flag is passed. This is because it would cause inconsistencies in heap dumps when trace events gets dropped between tracing sessions. This CL changes the TraceConfig so that the trcae config instance held by TraceLog will always have heap profiler filter predicate included, irrespective of enabling and disabling tracing. BUG=625170 ========== to ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added by both RECORDING_MODE config and FILTERING_MODE config. But, once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 ==========
Patchset #2 (id:20001) has been deleted
made changes as discussed, ptal, thanks.
The CQ bit was checked by ssid@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ssid@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by ssid@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.
This is looking much better to me! A couple of small comments, but I'd like primiano to sanity check this as well since the filtering thing has been evolving a fair bit now. https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:99: bool IsEnabled() { return mode_ & RECORDING_MODE; } I think we should rename this to IsEnabledForRecording() now. https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:102: bool IsFilteringEnabled(); What's the reason for not just checking for filtering mode here, like IsEnabled(ForRecording?) above? Doing an IsEnabled(Mode). https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:501: int mode_; // See TraceLog::Mode. nit: Since this is now a bitmask it should probably use a specific sized type like uint8_t instead.
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:99: bool IsEnabled() { return mode_ & RECORDING_MODE; } On 2016/09/22 19:21:06, oystein wrote: > I think we should rename this to IsEnabledForRecording() now. Do you mind if I do this as a separate clean up CL? I would have to change some 20 files where this is being used. multiple owners will get spammed by discussion in this CL. https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:102: bool IsFilteringEnabled(); On 2016/09/22 19:21:06, oystein wrote: > What's the reason for not just checking for filtering mode here, like > IsEnabled(ForRecording?) above? Doing an IsEnabled(Mode). So, I really need to check if filters are enabled rather than if filtering mode is enabled. This is because if filters were enabled by any mode previously then we cannot add more filters since it will cause races on the filters vector. I added an additional function to get mode. https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:501: int mode_; // See TraceLog::Mode. On 2016/09/22 19:21:06, oystein wrote: > nit: Since this is now a bitmask it should probably use a specific sized type > like uint8_t instead. Done.
Great! lgtm pending primiano sanity-checking. https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:99: bool IsEnabled() { return mode_ & RECORDING_MODE; } On 2016/09/22 19:37:43, ssid wrote: > On 2016/09/22 19:21:06, oystein wrote: > > I think we should rename this to IsEnabledForRecording() now. > > Do you mind if I do this as a separate clean up CL? I would have to change some > 20 files where this is being used. multiple owners will get spammed by > discussion in this CL. Yep that's fine. https://codereview.chromium.org/2323483005/diff/80001/base/trace_event/trace_... base/trace_event/trace_log.h:102: bool IsFilteringEnabled(); On 2016/09/22 19:37:43, ssid wrote: > On 2016/09/22 19:21:06, oystein wrote: > > What's the reason for not just checking for filtering mode here, like > > IsEnabled(ForRecording?) above? Doing an IsEnabled(Mode). > > So, I really need to check if filters are enabled rather than if filtering mode > is enabled. This is because if filters were enabled by any mode previously then > we cannot add more filters since it will cause races on the filters vector. > I added an additional function to get mode. Okay, that makes sense.I think I'd avoid "Enabled" as part of the name in that case though, to make it clear it's not an equivalent function to IsEnabled??(). "HaveActiveFilters" maybe, up to you! https://codereview.chromium.org/2323483005/diff/120001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/2323483005/diff/120001/base/trace_event/trace... base/trace_event/trace_config.h:248: void SetEventFilterConfigs(EventFilters filter_configs) { const EventFilters&?
ok conceptually the two independent modes make sense. I have a bunch of questions and some small suggestion in the meantime. The only thing that is a bit clunky to think about is all the logic around the filter list. I need to think again with fresh mind to see if we can do something simpler. One thing could help was if this commit message did explain the actual use cases. In which case do we expect first ENABLE_FOR_RECORDING and then ENABLE_FOR_FILTERING? In which viceversa? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:134: << ", event in stack: " << pseudo_stack_.back().trace_event_name; I'd probably s/,/vs/ https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:209: !TraceLog::GetInstance()->HaveActiveFilters()) { can you expand what is the sense of the HaveActiveFilters() check here? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:734: void TraceLog::SetEnabled(const TraceConfig& trace_config, Mode new_mode) { add DCHECK to |new_mode| being exclusive and not or-ed (See other comment, sorry for commenting out of order) https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:771: } what happens if tracing is already enabled for FILTERING_MODE and then you enable for TRACING passing extra filters? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:886: void TraceLog::SetDisabledWhileLocked(Mode mode) { can you SetDisabled(RECORDING | FILTERING) ? If not add a DCHECK(mode & (mode - 1); ( mode being a pow of two) here and in SetEnabled. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:899: // Stop the sampling thread. should all this stuff happen even if you are just disabling FILTERING mode? I think this is relevant only for recording, right? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:908: enabled_modes_ &= ~mode; is there a reason why you moved this statement before the if {} block above? I wonder if any of those dtors make any assumptions on the mode. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:909: if (mode == RECORDING_MODE) should this be s/==/&/ ? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:912: if (!mode || mode == FILTERING_MODE) In which case mode is expected to be == 0? I though you always pass somethign here. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.h:49: enum Mode { RECORDING_MODE = 1 << 0, FILTERING_MODE = 1 << 1 }; enum Mode : uint8_t { ... } to match the uint8_t below
Mainly 2 comments: Filters cannot be added when filtering is enabled. One improvement i could do here for the startup tracing case is try to keep the recording mode filters on for the filtering mode. But, it feels very bad for TraceLog to support this. We can think about it later if needed. Do i need to sanity check the Enum argument passed, so that 2 values are not passed together? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:134: << ", event in stack: " << pseudo_stack_.back().trace_event_name; On 2016/09/23 18:34:31, Primiano Tucci wrote: > I'd probably s/,/vs/ Done. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:209: !TraceLog::GetInstance()->HaveActiveFilters()) { On 2016/09/23 18:34:31, Primiano Tucci wrote: > can you expand what is the sense of the HaveActiveFilters() check here? So, filters are a list and adding more objects to list while trace events are accessing this list from other threads causes races. So, adding / removing filters while filters were enabled is not supported. This would happen if startup tracing config has filters enabled and this filters will not get added. So, we can't support the case where we start with startup tracing and try tracing again with chrome://tracing. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:734: void TraceLog::SetEnabled(const TraceConfig& trace_config, Mode new_mode) { On 2016/09/23 18:34:32, Primiano Tucci wrote: > add DCHECK to |new_mode| being exclusive and not or-ed (See other comment, sorry > for commenting out of order) Do I need this dcheck even when passing an enum argument? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:771: } On 2016/09/23 18:34:32, Primiano Tucci wrote: > what happens if tracing is already enabled for FILTERING_MODE and then you > enable for TRACING passing extra filters? I did not want to cause race conditions on the std::vector by adding more filters to the list. Ideally I could use an array and add more elements to the array of filters, but then I would also have to keep track of which filters were enabled by recording_mode and which filters were enabled by filtering_mode, so that after recording_mode i will keep the filtering_mode filters enabled. To avoid this complication i do not support adding / removing filters when filtering is enabled by any mode. So, if filters were added by recording_mode when filtering_mode was already enabled, then we ignore those filters and use the existing filters. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:886: void TraceLog::SetDisabledWhileLocked(Mode mode) { On 2016/09/23 18:34:32, Primiano Tucci wrote: > can you SetDisabled(RECORDING | FILTERING) ? > If not add a DCHECK(mode & (mode - 1); ( mode being a pow of two) here and in > SetEnabled. Um, the argument says "Mode" and not uint8_t. I think it is clear that the user should pass only only mode right? https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:899: // Stop the sampling thread. On 2016/09/23 18:34:32, Primiano Tucci wrote: > should all this stuff happen even if you are just disabling FILTERING mode? > I think this is relevant only for recording, right? Done. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:908: enabled_modes_ &= ~mode; On 2016/09/23 18:34:31, Primiano Tucci wrote: > is there a reason why you moved this statement before the if {} block above? I > wonder if any of those dtors make any assumptions on the mode. Done. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:909: if (mode == RECORDING_MODE) On 2016/09/23 18:34:32, Primiano Tucci wrote: > should this be s/==/&/ ? mode is enum value. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.cc:912: if (!mode || mode == FILTERING_MODE) On 2016/09/23 18:34:32, Primiano Tucci wrote: > In which case mode is expected to be == 0? I though you always pass somethign > here. Sorry, mixed mode and mode_ here when i renamed mode_ to enabled_modes_. https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/140001/base/trace_event/trace... base/trace_event/trace_log.h:49: enum Mode { RECORDING_MODE = 1 << 0, FILTERING_MODE = 1 << 1 }; On 2016/09/23 18:34:32, Primiano Tucci wrote: > enum Mode : uint8_t { ... } > to match the uint8_t below Done.
Description was changed from ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added by both RECORDING_MODE config and FILTERING_MODE config. But, once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 ========== to ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added only by FILTERING_MODE and trace config. Once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 ==========
Patchset #6 (id:180001) has been deleted
ptal, thanks
The CQ bit was checked by ssid@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Ok I have only some smaller comments but this LG. Will do a second pass tomorrow with fresh mind. Oystein, in the meantime mind having another pass? Please look at the comments below, some of them are quite important (especially the if (!enabled_modes_ & FILTERING) one) https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc:40: " \"included_categories\": [\"disabled-by-default-memory-infra\"]" should this be: -*, disabled-.... otherwise all the default categories are still enabled by default. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:204: TRACE_EVENT0(kTraceCategory, "init"); // Add to trace-viewer category list. I learned only recently that there is a macro for this: TRACE_EVENT_WARMUP_CATEGORY(kTraceCategory) while cleaning up things :) https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:209: // TraceLog. So, in case of startup tracing filtering mode is not enabled. s/startup/background/ I think? https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:211: !(TraceLog::GetInstance()->enabled_modes() & TraceLog::FILTERING_MODE)) { shouldn't you do all this only if enabled for pseudo stack, or do you want to support hybrid modes soon? https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:214: heap_profiler_filter_config.AddIncludedCategory("*"); should you also include here disabled-by-default-* ? maybe in a separate patch as we never tried that before :) https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:216: MemoryDumpManager::kTraceCategory); out of curiosity why do yo need to add memory-infra to the filter category list itself? https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_config.cc:122: void TraceConfig::MemoryDumpConfig::Merge( just to be clear, this is an indipendent fix? It was broken from previous CLs, right? https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3261: EXPECT_EQ(TraceLog::FILTERING_MODE, TraceLog::GetInstance()->enabled_modes()); can you add some comments every time you do setenabled/disabled to explain what do you expect to happen? Thanks https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3262: { why there are scope enclosers here? is it just for readability (in which case fine)? or is there some other subtle thing going on? https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3273: { TRACE_EVENT0("c2", "name2"); } here either use newlines (e.g. {\nTRACE...\n}) or drop the braces. don't feel this adds much readability for one line https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (left): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1349: InitializeThreadLocalEventBufferIfSupported(); hmm can you keep the InitializeTLEBIS here (maybe behind a if(mode & recording) ? I remember I had to add this because was causing problems when trying to use memory-infra when creating the TLS too late. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:570: enabled_flag |= ENABLED_FOR_EVENT_CALLBACK; I thought ENABLED_FOR_EVENT_CALLBACK was gone? or maybe I am confusing some older cl? (not a blocker for this cl though) https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:612: if (!enabled_modes_ & FILTERING_MODE) watch out operator orders, here you really want if (!(enabled_modes_ & FILTERING)) thankfully MSVC warned about this. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:760: bool already_recording = enabled_modes_ & RECORDING_MODE; +const (const bool) https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:86: // will be traced. SetDisabled must be called for each mode that is enabled. s/for each mode that is enabled./distinctly for each mode (recording or filtering)/ https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:87: // If tracing has already been enabled, category filter (enabled and disabled s/enabled/enabled for recording/ https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:91: // only when FILTERING_MODE is disabled. I see what you mean here with this last sentence but I think should be worded better, e.g.: Conversely to RECORDING_MODE, FILTERING_MODE doesn't support upgrading, i.e. filters can only be enabled if not previously enabled. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:104: bool IsEnabled() { return enabled_modes_ & RECORDING_MODE; } fortunately we don't have too many cases for this. In a follow up patch can you please make this bool IsEnabledFor(Mode mode) { return enabled_modes_ & mode) and change the few call sites? Thisw ill bring a bit more of sanity :)
Thanks, fixed / replied to comments. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc:40: " \"included_categories\": [\"disabled-by-default-memory-infra\"]" On 2016/10/12 17:50:17, Primiano Tucci wrote: > should this be: -*, disabled-.... > otherwise all the default categories are still enabled by default. Actually I think the SetEnabled must not have RECORDING_MODE. fixed that. and removed this. I do not feel the need to test that recording_mode should still have filters on, which is tested in other unittests https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:204: TRACE_EVENT0(kTraceCategory, "init"); // Add to trace-viewer category list. On 2016/10/12 17:50:17, Primiano Tucci wrote: > I learned only recently that there is a macro for this: > > TRACE_EVENT_WARMUP_CATEGORY(kTraceCategory) > > while cleaning up things :) Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:209: // TraceLog. So, in case of startup tracing filtering mode is not enabled. On 2016/10/12 17:50:17, Primiano Tucci wrote: > s/startup/background/ I think? Actually this is not true. The first comment takes care of all the issues. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:211: !(TraceLog::GetInstance()->enabled_modes() & TraceLog::FILTERING_MODE)) { On 2016/10/12 17:50:17, Primiano Tucci wrote: > shouldn't you do all this only if enabled for pseudo stack, or do you want to > support hybrid modes soon? Sorry this was mistake. That will be different patch. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:214: heap_profiler_filter_config.AddIncludedCategory("*"); On 2016/10/12 17:50:17, Primiano Tucci wrote: > should you also include here disabled-by-default-* ? maybe in a separate patch > as we never tried that before :) Currently we enable disabled-by-default categories by default for filtering. So, this is not required. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:216: MemoryDumpManager::kTraceCategory); On 2016/10/12 17:50:17, Primiano Tucci wrote: > out of curiosity why do yo need to add memory-infra to the filter category list > itself? In case in future we decide to have enabled-categories for filters like recording mode categories, then we might not filter disabled-by-default categories. If this filter does not return true for memory-infra and there is some otehr filter that returns false for memory-infra then we will miss the memory dumps from tracing. So, this makes sure that the dumps are added. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_config.cc:122: void TraceConfig::MemoryDumpConfig::Merge( On 2016/10/12 17:50:17, Primiano Tucci wrote: > just to be clear, this is an indipendent fix? It was broken from previous CLs, > right? Yes independent since i just realized Merge is being called at SetEnabled https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3261: EXPECT_EQ(TraceLog::FILTERING_MODE, TraceLog::GetInstance()->enabled_modes()); On 2016/10/12 17:50:17, Primiano Tucci wrote: > can you add some comments every time you do setenabled/disabled to explain what > do you expect to happen? > Thanks Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3262: { On 2016/10/12 17:50:17, Primiano Tucci wrote: > why there are scope enclosers here? is it just for readability (in which case > fine)? or is there some other subtle thing going on? I am measuring the end_event_hit_count. To check the number of EndEvent calls is consistent. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_event_unittest.cc:3273: { TRACE_EVENT0("c2", "name2"); } On 2016/10/12 17:50:17, Primiano Tucci wrote: > here either use newlines (e.g. {\nTRACE...\n}) or drop the braces. don't feel > this adds much readability for one line Um, this is a fight against git cl format :( https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (left): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:1349: InitializeThreadLocalEventBufferIfSupported(); On 2016/10/12 17:50:17, Primiano Tucci wrote: > hmm can you keep the InitializeTLEBIS here (maybe behind a if(mode & recording) > ? > I remember I had to add this because was causing problems when trying to use > memory-infra when creating the TLS too late. Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:570: enabled_flag |= ENABLED_FOR_EVENT_CALLBACK; On 2016/10/12 17:50:17, Primiano Tucci wrote: > I thought ENABLED_FOR_EVENT_CALLBACK was gone? or maybe I am confusing some > older cl? (not a blocker for this cl though) Sorry, i still did not land that one. that is waiting on this one because of my laziness to rebase. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:612: if (!enabled_modes_ & FILTERING_MODE) On 2016/10/12 17:50:17, Primiano Tucci wrote: > watch out operator orders, here you really want > if (!(enabled_modes_ & FILTERING)) > > thankfully MSVC warned about this. yes, i was going to fix this. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.cc:760: bool already_recording = enabled_modes_ & RECORDING_MODE; On 2016/10/12 17:50:17, Primiano Tucci wrote: > +const (const bool) Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:86: // will be traced. SetDisabled must be called for each mode that is enabled. On 2016/10/12 17:50:18, Primiano Tucci wrote: > s/for each mode that is enabled./distinctly for each mode (recording or > filtering)/ Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:87: // If tracing has already been enabled, category filter (enabled and disabled On 2016/10/12 17:50:18, Primiano Tucci wrote: > s/enabled/enabled for recording/ Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:91: // only when FILTERING_MODE is disabled. On 2016/10/12 17:50:18, Primiano Tucci wrote: > I see what you mean here with this last sentence but I think should be worded > better, e.g.: > Conversely to RECORDING_MODE, FILTERING_MODE doesn't support upgrading, i.e. > filters can only be enabled if not previously enabled. Done. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/trace... base/trace_event/trace_log.h:104: bool IsEnabled() { return enabled_modes_ & RECORDING_MODE; } On 2016/10/12 17:50:17, Primiano Tucci wrote: > fortunately we don't have too many cases for this. > In a follow up patch can you please make this > > bool IsEnabledFor(Mode mode) { return enabled_modes_ & mode) and change the few > call sites? > Thisw ill bring a bit more of sanity :) Yes, that is why I added a TODO on top to remove this function
The CQ bit was checked by ssid@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...
Thanks for this! A few nits only basically. https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:216: MemoryDumpManager::kTraceCategory); On 2016/10/12 19:04:43, ssid wrote: > On 2016/10/12 17:50:17, Primiano Tucci wrote: > > out of curiosity why do yo need to add memory-infra to the filter category > list > > itself? > > In case in future we decide to have enabled-categories for filters like > recording mode categories, then we might not filter disabled-by-default > categories. If this filter does not return true for memory-infra and there is > some otehr filter that returns false for memory-infra then we will miss the > memory dumps from tracing. So, this makes sure that the dumps are added. If this is just in-case we change this in the future, I would just add a comment about it instead. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:220: TraceLog::GetInstance()->SetEnabled(filtering_trace_config, bikeshedding readability nit: Maybe a break here and a comment on top of the above block that states this is just prepping the config, to make it immediately obvious that's the main thing happening here. // Configure the filtering TraceConfig TraceConfig filtering_trace_config; ... do stuff to filtering_trace_config; TraceLog::GetInstance()->SetEnabled(filtering_trace_config, TraceLog::FILTERING_MODE); https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:226: bool tracing_already_enabled = TraceLog::GetInstance()->IsEnabled(); nit and not about this CL (but since you're touching it :): Why isn't the IsEnabled() call just inlined in the if() two lines below? The current ordering makes it seem like AddEnabledStateObserver() can have side-effects in terms of enabling the TraceLog, which it can't. Or is this some threading thing? Deserves a comment if so. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_config.h:245: void SetEventFilterConfigs(const EventFilters& filter_configs) { nit: SetEventFilters, for consistent naming (also 'Config' is somewhat redundant in a config class). https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.cc:763: DCHECK_EQ(new_options, old_options) << "Attempting to re-enable " Add a TODO to remove this (i.e. changing options run-time), if we can, and move to a priority system instead. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.h:49: // RECORDING_MODE: Enables normal tracing (recording trace events in the nit: No need for the 'RECORDING_MODE' and 'FILTERING_MODE' parts, it's obvious when the comment is on the line above the enum entries. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.h:85: // See TraceConfig comments for details on how to control what categories will nit: s/what/which/ https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.h:541: TraceConfig::EventFilters event_filters_enabled_; 'enabled_event_filters_' maybe, current phrasing makes it sound like a bool which controls whether event filters in general are enabled or not, rather than *which* event filters are enabled.
Patchset #9 (id:260001) has been deleted
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by primiano@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ssid@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...
Thanks, fixed. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:220: TraceLog::GetInstance()->SetEnabled(filtering_trace_config, On 2016/10/13 00:34:29, oystein wrote: > bikeshedding readability nit: Maybe a break here and a comment on top of the > above block that states this is just prepping the config, to make it immediately > obvious that's the main thing happening here. > > // Configure the filtering TraceConfig > TraceConfig filtering_trace_config; > ... do stuff to filtering_trace_config; > > TraceLog::GetInstance()->SetEnabled(filtering_trace_config, > TraceLog::FILTERING_MODE); Done. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:226: bool tracing_already_enabled = TraceLog::GetInstance()->IsEnabled(); On 2016/10/13 00:34:29, oystein wrote: > nit and not about this CL (but since you're touching it :): Why isn't the > IsEnabled() call just inlined in the if() two lines below? The current ordering > makes it seem like AddEnabledStateObserver() can have side-effects in terms of > enabling the TraceLog, which it can't. Or is this some threading thing? Deserves > a comment if so. Um, this is done here so that we do not call OnTraceLogEnabled twice. There could be a case where: thread 1: AddObserver thread 1: IsEnabled -> false thread 2: SetEnabled and observer->OnTraceLogEnabled thread 1: OnTraceLogEnabled // since the previous IsEnabled returned false. This could cause double initialization of the dump timer which is worse than in few cases here which will not enable timer at all. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_config.h:245: void SetEventFilterConfigs(const EventFilters& filter_configs) { On 2016/10/13 00:34:30, oystein wrote: > nit: SetEventFilters, for consistent naming (also 'Config' is somewhat redundant > in a config class). SetEventFilters already exists in this class. Fixed it. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.cc:763: DCHECK_EQ(new_options, old_options) << "Attempting to re-enable " On 2016/10/13 00:34:30, oystein wrote: > Add a TODO to remove this (i.e. changing options run-time), if we can, and move > to a priority system instead. Not sure I understand priority system. Added a todo. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.h:49: // RECORDING_MODE: Enables normal tracing (recording trace events in the On 2016/10/13 00:34:30, oystein wrote: > nit: No need for the 'RECORDING_MODE' and 'FILTERING_MODE' parts, it's obvious > when the comment is on the line above the enum entries. Done. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.h:85: // See TraceConfig comments for details on how to control what categories will On 2016/10/13 00:34:30, oystein wrote: > nit: s/what/which/ Done. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/trace... base/trace_event/trace_log.h:541: TraceConfig::EventFilters event_filters_enabled_; On 2016/10/13 00:34:30, oystein wrote: > 'enabled_event_filters_' maybe, current phrasing makes it sound like a bool > which controls whether event filters in general are enabled or not, rather than > *which* event filters are enabled. Done.
The CQ bit was checked by primiano@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...
Allright, LTGM Thanks a lot for the hard work here. https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:226: bool tracing_already_enabled = TraceLog::GetInstance()->IsEnabled(); On 2016/10/13 17:30:54, ssid wrote: > On 2016/10/13 00:34:29, oystein wrote: > > nit and not about this CL (but since you're touching it :): Why isn't the > > IsEnabled() call just inlined in the if() two lines below? The current > ordering > > makes it seem like AddEnabledStateObserver() can have side-effects in terms of > > enabling the TraceLog, which it can't. Or is this some threading thing? > Deserves > > a comment if so. > > Um, this is done here so that we do not call OnTraceLogEnabled twice. > There could be a case where: > thread 1: AddObserver > thread 1: IsEnabled -> false > thread 2: SetEnabled and observer->OnTraceLogEnabled > thread 1: OnTraceLogEnabled // since the previous IsEnabled returned false. > > This could cause double initialization of the dump timer which is worse than in > few cases here which will not enable timer at all. Correct ;-) https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... base/trace_event/trace_log.cc:870: void TraceLog::SetDisabledWhileLocked(uint8_t modes) { nit: probably a bit easier to read if this var is called modes_to_disable My brain tripped twice while reading this because I didn't realize that the bits in modes and enabled_modes_ have the opposite meaning (modes == RECORDING -> I want to disable recording) https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... base/trace_event/trace_log.h:47: // Argument passed to TrcaeLog::SetEnabled. typo: TraceLog (But if the rest is fine, whatever, I'm going to cleanup this file for a bit)
On 2016/10/13 19:11:24, Primiano Tucci wrote: > Allright, LTGM haha typo :D
lgtm! https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2323483005/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:226: bool tracing_already_enabled = TraceLog::GetInstance()->IsEnabled(); On 2016/10/13 17:30:54, ssid wrote: > On 2016/10/13 00:34:29, oystein wrote: > > nit and not about this CL (but since you're touching it :): Why isn't the > > IsEnabled() call just inlined in the if() two lines below? The current > ordering > > makes it seem like AddEnabledStateObserver() can have side-effects in terms of > > enabling the TraceLog, which it can't. Or is this some threading thing? > Deserves > > a comment if so. > > Um, this is done here so that we do not call OnTraceLogEnabled twice. > There could be a case where: > thread 1: AddObserver > thread 1: IsEnabled -> false > thread 2: SetEnabled and observer->OnTraceLogEnabled > thread 1: OnTraceLogEnabled // since the previous IsEnabled returned false. > > This could cause double initialization of the dump timer which is worse than in > few cases here which will not enable timer at all. Yep makes sense; comment still needed I think :)
Patchset #12 (id:360001) has been deleted
Done. thanks https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... base/trace_event/trace_log.cc:870: void TraceLog::SetDisabledWhileLocked(uint8_t modes) { On 2016/10/13 19:11:24, Primiano Tucci wrote: > nit: probably a bit easier to read if this var is called modes_to_disable > My brain tripped twice while reading this because I didn't realize that the bits > in modes and enabled_modes_ have the opposite meaning (modes == RECORDING -> I > want to disable recording) Done. https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2323483005/diff/340001/base/trace_event/trace... base/trace_event/trace_log.h:47: // Argument passed to TrcaeLog::SetEnabled. On 2016/10/13 19:11:24, Primiano Tucci wrote: > typo: TraceLog (But if the rest is fine, whatever, I'm going to cleanup this > file for a bit) Done.
The CQ bit was checked by ssid@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...
lol, I meant LGTM
Patchset #1 (id:1) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/2323483005/#ps380001 (title: "Add a comment, and renames.")
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.
Description was changed from ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added only by FILTERING_MODE and trace config. Once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 ========== to ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added only by FILTERING_MODE and trace config. Once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added only by FILTERING_MODE and trace config. Once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 ========== to ========== [tracing] Add filtering mode in TraceLog This CL does: 1. Add new FILTERING_MODE in TraceLog. TraceLog can be enabled and disabled with FILTERING_MODE independently from RECORDING MODE. The TraceLog::mode_ is a bitmap. 2. In FILTERING_MODE trace buffer is not created and trace events are passed to filters but not recorded. 3. The filters can be added only by FILTERING_MODE and trace config. Once enabled, the filters cannot be changed until the filters are disabled. 4. HeapProfilerFilter is enabled using FILTERING_MODE when "--enable-heap-profiling" flag is passed, so that pseudo stack is recorded for all allocations, irrespective of tracing. BUG=625170 Committed: https://crrev.com/20d2eeed2c735fbf018b9a9dffa810d51a25940d Cr-Commit-Position: refs/heads/master@{#425156} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/20d2eeed2c735fbf018b9a9dffa810d51a25940d Cr-Commit-Position: refs/heads/master@{#425156}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:380001) has been created in https://codereview.chromium.org/2415183004/ by kulshin@chromium.org. The reason for reverting is: Causes webkit failures https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Pre... in https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
Message was sent while issue was closed.
Sheriffs: If there are more build breaks due to this CL, please contact primiano@ or oysteine@ as I will be on vacation next 2 weeks. Reverting this CL would mean reverting multiple CLs that landed after this one as listed in crbug.com/625170. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
