Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739163004:
[tracing] Use same code path for category filtering for recording and event filtering (Closed)
Description[tracing] Use same code path for category filtering for recording and event filtering
Previously there was not way to filter just default categories using the
category filtering logic of event filters. This CL refactors the
category filtering logic out of TraceConfig and both EventFilterConfig
and TraceConfig uses this new class.
BUG=700245
Review-Url: https://codereview.chromium.org/2739163004
Cr-Commit-Position: refs/heads/master@{#459557}
Committed: https://chromium.googlesource.com/chromium/src/+/114dc8babbde99d54f3f4f41246a19bb1a4c1605
Patch Set 1 #Patch Set 2 : nit. #Patch Set 3 : Refactor category filter config. #Patch Set 4 : with similarity=20 #
Total comments: 5
Patch Set 5 : InitalizeWithString. #
Messages
Total messages: 37 (20 generated)
ssid@chromium.org changed reviewers: + oysteine@chromium.org
Oystein, wdyt?
Description was changed from ========== Do not filter disabled-by-default categories by default The heap profiler needs a way to enable default categories and just disable disabled categories, but also to whitelist some disabled categories. To achieve this the disabled-by-default categories will not be filtered by default and held as a seperate list. Adding "*" in included categories will filter enabled categories and adding "disabled-by-default-*" will filter all disabled categories. BUG=700245 ========== to ========== [tracing] Do not filter disabled-by-default categories by default The heap profiler needs a way to enable default categories and just disable disabled categories, but also to whitelist some disabled categories. To achieve this the disabled-by-default categories will not be filtered by default and held as a seperate list. Adding "*" in included categories will filter enabled categories and adding "disabled-by-default-*" will filter all disabled categories. BUG=700245 ==========
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/03/10 04:19:18, ssid wrote: > Oystein, wdyt? @oysteine ping.
The logic of the categoryfilter stuff is already super convoluted, I really don't think we should add another layer into the mix :/. Why can't you achieve the same thing by doing something like disabled-by-default-*,-cc,-net?
On 2017/03/16 at 02:00:24, oystein wrote: > The logic of the categoryfilter stuff is already super convoluted, I really don't think we should add another layer into the mix :/. Why can't you achieve the same thing by doing something like disabled-by-default-*,-cc,-net? or rather disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net that is.
On 2017/03/16 02:01:02, oystein wrote: > On 2017/03/16 at 02:00:24, oystein wrote: > > The logic of the categoryfilter stuff is already super convoluted, I really > don't think we should add another layer into the mix :/. Why can't you achieve > the same thing by doing something like disabled-by-default-*,-cc,-net? > > or rather disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net > that is. So the issue is this: We want all enabled categories to be filtered. We want only some disabled categories to be filtered. Cases right now: category_filter="*, -disabled-by-default-*, disabled-by-default-net": This would enable all categories and disabled disabled-by-default- categories. But, it cannot enable the net category because the check for excluded categories happens first and net is not enabled. category_filter="*, disabled-by-default-net": This would enable all categories including disabled ones, which we do not want. Why I think this complexity is ok is because the category filter matching is going to replicate the category filter matching code for recording mode: https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?type=cs... Maybe I can think of a way for both of these to use the same code. But, I somehow thought that Primiano was trying to move the filters code separately, and using the same function might cause complexity there.
On 2017/03/16 18:24:56, ssid wrote: > On 2017/03/16 02:01:02, oystein wrote: > > On 2017/03/16 at 02:00:24, oystein wrote: > > > The logic of the categoryfilter stuff is already super convoluted, I really > > don't think we should add another layer into the mix :/. Why can't you achieve > > the same thing by doing something like disabled-by-default-*,-cc,-net? > > > > or rather > disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net > > that is. > > So the issue is this: > We want all enabled categories to be filtered. We want only some disabled > categories to be filtered. > > Cases right now: > category_filter="*, -disabled-by-default-*, disabled-by-default-net": This would > enable all categories and disabled disabled-by-default- categories. But, it > cannot enable the net category because the check for excluded categories happens > first and net is not enabled. > > category_filter="*, disabled-by-default-net": This would enable all categories > including disabled ones, which we do not want. if this is really the problem, I think the right filter here is "disabled-by-default-net" (I know the category filter is awkward but this is the way it is)
On 2017/03/16 18:35:38, Primiano Tucci (slow - perf) wrote: > On 2017/03/16 18:24:56, ssid wrote: > > On 2017/03/16 02:01:02, oystein wrote: > > > On 2017/03/16 at 02:00:24, oystein wrote: > > > > The logic of the categoryfilter stuff is already super convoluted, I > really > > > don't think we should add another layer into the mix :/. Why can't you > achieve > > > the same thing by doing something like disabled-by-default-*,-cc,-net? > > > > > > or rather > > disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net > > > that is. > > > > So the issue is this: > > We want all enabled categories to be filtered. We want only some disabled > > categories to be filtered. > > > > Cases right now: > > category_filter="*, -disabled-by-default-*, disabled-by-default-net": This > would > > enable all categories and disabled disabled-by-default- categories. But, it > > cannot enable the net category because the check for excluded categories > happens > > first and net is not enabled. > > > > category_filter="*, disabled-by-default-net": This would enable all categories > > including disabled ones, which we do not want. > > if this is really the problem, I think the right filter here is > "disabled-by-default-net" (I know the category filter is awkward but this is the > way it is) Um that still does not solve the issue here. Either we will be able to get all categories including disabled or we cannot get enabled categories because we can't include "*". One solution can be to list all default categories by name that has to be enabled in MDM.
On 2017/03/16 18:42:41, ssid wrote: > On 2017/03/16 18:35:38, Primiano Tucci (slow - perf) wrote: > > On 2017/03/16 18:24:56, ssid wrote: > > > On 2017/03/16 02:01:02, oystein wrote: > > > > On 2017/03/16 at 02:00:24, oystein wrote: > > > > > The logic of the categoryfilter stuff is already super convoluted, I > > really > > > > don't think we should add another layer into the mix :/. Why can't you > > achieve > > > > the same thing by doing something like disabled-by-default-*,-cc,-net? > > > > > > > > or rather > > > disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net > > > > that is. > > > > > > So the issue is this: > > > We want all enabled categories to be filtered. We want only some disabled > > > categories to be filtered. > > > > > > Cases right now: > > > category_filter="*, -disabled-by-default-*, disabled-by-default-net": This > > would > > > enable all categories and disabled disabled-by-default- categories. But, it > > > cannot enable the net category because the check for excluded categories > > happens > > > first and net is not enabled. > > > > > > category_filter="*, disabled-by-default-net": This would enable all > categories > > > including disabled ones, which we do not want. > > > > if this is really the problem, I think the right filter here is > > "disabled-by-default-net" (I know the category filter is awkward but this is > the > > way it is) > > Um that still does not solve the issue here. Either we will be able to get all > categories including disabled or we cannot get enabled categories because we > can't include "*". One solution can be to list all default categories by name > that has to be enabled in MDM. Are you sure? If no enabled-by-default categories are explicitly specified, the default is to include all enabled-by-default ones. Since "disabled-by-default-net" omits that, and had no exclusions it *should* work?
On 2017/03/16 19:34:02, oysteine wrote: > On 2017/03/16 18:42:41, ssid wrote: > > On 2017/03/16 18:35:38, Primiano Tucci (slow - perf) wrote: > > > On 2017/03/16 18:24:56, ssid wrote: > > > > On 2017/03/16 02:01:02, oystein wrote: > > > > > On 2017/03/16 at 02:00:24, oystein wrote: > > > > > > The logic of the categoryfilter stuff is already super convoluted, I > > > really > > > > > don't think we should add another layer into the mix :/. Why can't you > > > achieve > > > > > the same thing by doing something like disabled-by-default-*,-cc,-net? > > > > > > > > > > or rather > > > > disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net > > > > > that is. > > > > > > > > So the issue is this: > > > > We want all enabled categories to be filtered. We want only some disabled > > > > categories to be filtered. > > > > > > > > Cases right now: > > > > category_filter="*, -disabled-by-default-*, disabled-by-default-net": This > > > would > > > > enable all categories and disabled disabled-by-default- categories. But, > it > > > > cannot enable the net category because the check for excluded categories > > > happens > > > > first and net is not enabled. > > > > > > > > category_filter="*, disabled-by-default-net": This would enable all > > categories > > > > including disabled ones, which we do not want. > > > > > > if this is really the problem, I think the right filter here is > > > "disabled-by-default-net" (I know the category filter is awkward but this is > > the > > > way it is) > > > > Um that still does not solve the issue here. Either we will be able to get all > > categories including disabled or we cannot get enabled categories because we > > can't include "*". One solution can be to list all default categories by name > > that has to be enabled in MDM. > > Are you sure? If no enabled-by-default categories are explicitly specified, the > default is to include all enabled-by-default ones. Since > "disabled-by-default-net" omits that, and had no exclusions it *should* work? This is the case with the filter_string parsing for recording mode. But, not the case with the filter category filter string parsing. The latter does not take into account disabled-by-default categories at all. That is what this CL is trying to fix. I think this code for filter's category string parsing should now match the code here: https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?type=cs... and https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?type=cs... This one has too much logic here which i think is not fully relevant for filters. Something about synthetic delays. But, I can try to extract a function out of these 2 if you feel that looks better.
On 2017/03/16 at 19:40:45, ssid wrote: > On 2017/03/16 19:34:02, oysteine wrote: > > On 2017/03/16 18:42:41, ssid wrote: > > > On 2017/03/16 18:35:38, Primiano Tucci (slow - perf) wrote: > > > > On 2017/03/16 18:24:56, ssid wrote: > > > > > On 2017/03/16 02:01:02, oystein wrote: > > > > > > On 2017/03/16 at 02:00:24, oystein wrote: > > > > > > > The logic of the categoryfilter stuff is already super convoluted, I > > > > really > > > > > > don't think we should add another layer into the mix :/. Why can't you > > > > achieve > > > > > > the same thing by doing something like disabled-by-default-*,-cc,-net? > > > > > > > > > > > > or rather > > > > > disabled-by-default-*,-disabled-by-default-cc,-disabled-by-default-net > > > > > > that is. > > > > > > > > > > So the issue is this: > > > > > We want all enabled categories to be filtered. We want only some disabled > > > > > categories to be filtered. > > > > > > > > > > Cases right now: > > > > > category_filter="*, -disabled-by-default-*, disabled-by-default-net": This > > > > would > > > > > enable all categories and disabled disabled-by-default- categories. But, > > it > > > > > cannot enable the net category because the check for excluded categories > > > > happens > > > > > first and net is not enabled. > > > > > > > > > > category_filter="*, disabled-by-default-net": This would enable all > > > categories > > > > > including disabled ones, which we do not want. > > > > > > > > if this is really the problem, I think the right filter here is > > > > "disabled-by-default-net" (I know the category filter is awkward but this is > > > the > > > > way it is) > > > > > > Um that still does not solve the issue here. Either we will be able to get all > > > categories including disabled or we cannot get enabled categories because we > > > can't include "*". One solution can be to list all default categories by name > > > that has to be enabled in MDM. > > > > Are you sure? If no enabled-by-default categories are explicitly specified, the > > default is to include all enabled-by-default ones. Since > > "disabled-by-default-net" omits that, and had no exclusions it *should* work? > > This is the case with the filter_string parsing for recording mode. But, not the case with the filter category filter string parsing. The latter does not take into account disabled-by-default categories at all. That is what this CL is trying to fix. I think this code for filter's category string parsing should now match the code here: > https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?type=cs... > and > https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?type=cs... > > This one has too much logic here which i think is not fully relevant for filters. Something about synthetic delays. But, I can try to extract a function out of these 2 if you feel that looks better. Yeah if we're essentially converging to needing the same amount of logic for both types of category strings (modulo the synthetic delay weirdness), I think it'd be great to merge things together a little bit more if at all possible.
Description was changed from ========== [tracing] Do not filter disabled-by-default categories by default The heap profiler needs a way to enable default categories and just disable disabled categories, but also to whitelist some disabled categories. To achieve this the disabled-by-default categories will not be filtered by default and held as a seperate list. Adding "*" in included categories will filter enabled categories and adding "disabled-by-default-*" will filter all disabled categories. BUG=700245 ========== to ========== [tracing] Use same code path for category filtering for recording and event filtering Previously there was not way to filter just default categories using the category filtering logic of event filters. This CL refactors the category filtering logic out of TraceConfig and both EventFilterConfig and TraceConfig uses this new class. BUG=700245 ==========
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I split the category logic out of trace_config and used it for both trace config and event filter config. I have uploaded one patchset with similarity=20% hoping it is easier to review the new file. If not, please look at the previous patch which is identical except for the similarity ratio.
Thanks so much for doing this! Enthusiastic lgtm. https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... File base/trace_event/trace_config_category_filter.h (right): https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... base/trace_event/trace_config_category_filter.h:31: void InitializeFromStrings(const StringPiece& category_filter_string); nit: Singular 'String', since it's been changed to only take the one now. https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... base/trace_event/trace_config_category_filter.h:56: static bool IsCategoryNameAllowed(StringPiece str); nit: const StringPiece& (sorry, I know this was just moved around :P)
ssid@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for BUILD file change. ptal, Thanks
LGTM https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... File base/trace_event/trace_config_category_filter.h (right): https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... base/trace_event/trace_config_category_filter.h:56: static bool IsCategoryNameAllowed(StringPiece str); On 2017/03/24 17:59:25, oystein wrote: > nit: const StringPiece& (sorry, I know this was just moved around :P) The guidance for StringPiece is to pass it by value.
Thanks, fixed. https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... File base/trace_event/trace_config_category_filter.h (right): https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... base/trace_event/trace_config_category_filter.h:31: void InitializeFromStrings(const StringPiece& category_filter_string); On 2017/03/24 17:59:25, oystein wrote: > nit: Singular 'String', since it's been changed to only take the one now. Done. https://codereview.chromium.org/2739163004/diff/60001/base/trace_event/trace_... base/trace_event/trace_config_category_filter.h:56: static bool IsCategoryNameAllowed(StringPiece str); On 2017/03/24 18:12:47, dcheng wrote: > On 2017/03/24 17:59:25, oystein wrote: > > nit: const StringPiece& (sorry, I know this was just moved around :P) > > The guidance for StringPiece is to pass it by value. Thanks! I'll leave it as is.
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.
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, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2739163004/#ps80001 (title: "InitalizeWithString.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490390565181540,
"parent_rev": "d3fc0f37f5b506fbc57edfdeebe02cbeac5b3002", "commit_rev":
"114dc8babbde99d54f3f4f41246a19bb1a4c1605"}
Message was sent while issue was closed.
Description was changed from ========== [tracing] Use same code path for category filtering for recording and event filtering Previously there was not way to filter just default categories using the category filtering logic of event filters. This CL refactors the category filtering logic out of TraceConfig and both EventFilterConfig and TraceConfig uses this new class. BUG=700245 ========== to ========== [tracing] Use same code path for category filtering for recording and event filtering Previously there was not way to filter just default categories using the category filtering logic of event filters. This CL refactors the category filtering logic out of TraceConfig and both EventFilterConfig and TraceConfig uses this new class. BUG=700245 Review-Url: https://codereview.chromium.org/2739163004 Cr-Commit-Position: refs/heads/master@{#459557} Committed: https://chromium.googlesource.com/chromium/src/+/114dc8babbde99d54f3f4f41246a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/114dc8babbde99d54f3f4f41246a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
