So TRACE_EVENT_CATEGORY_GROUP_ENABLED() returns true, but category group is not really enabled?

What's the intended usage of TRACE_EVENT_CATEGORY_GROUP_ENABLED() then?

dskiba: The group is enabled for filtering but not recording, this case. Heap profiling essentially turns on *all* categories for filtering, which includes these heavy ones.

It's a bit ambiguous: Maybe there's cases where we'd want to emit these events for filtering and not just recording, but in cases where we're talking about something super-heavy like a screenshot we definitely won't.

We should rename this to TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING(), I guess. If there's actually a need for some of these conditionals to be recorded for both we could introduce more macros but I'm guessing there isn't.

Hmm by looking at the code looks like what I described shouldn't happen:

// Macro to efficiently determine if a given category group is enabled.
#define TRACE_EVENT_CATEGORY_GROUP_ENABLED(category_group, ret)             \
  do {                                                                      \
    INTERNAL_TRACE_EVENT_GET_CATEGORY_INFO(category_group);                 \
    if (INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE()) { \
      *ret = true;                                                          \
    } else {                                                                \
      *ret = false;                                                         \
    }                                                                       \
  } while (0)


so it seems that TRACE_EVENT_CATEGORY_GROUP_ENABLED() does already the right thing.
At the same time, I got from kraynov that we were hitting this line:
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_decoder.cc?rcl=0&l=11539

So hmm either I misunderstood kraynov's analysis or there is some bug and that macro is well-intended but not really doing the right thing. 

Ah there we go, just one step further

#define INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE() \
  UNLIKELY(*INTERNAL_TRACE_EVENT_UID(category_group_enabled) &           \
           (base::trace_event::TraceCategory::ENABLED_FOR_RECORDING |         \
            base::trace_event::TraceCategory::ENABLED_FOR_ETW_EXPORT |        \
            base::trace_event::TraceCategory::ENABLED_FOR_FILTERING))


so TRACE_EVENT_CATEGORY_GROUP_ENABLED is defined as (FOR_RECORDING | FOR_FILTERING).
Ok i need to think to this with fresh mind tomorrow morning. Any inputs welcome in the meantime.

Sorry for the delay. This is my fault. I did not realize that TRACE_EVENT_CATEGORY_GROUP_ENABLED internally uses INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE. I thought internal macro is not used outside.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ce3cd84139f6f60cb6a7458f0fbc95315241fd9

commit 1ce3cd84139f6f60cb6a7458f0fbc95315241fd9
Author: ssid <ssid@chromium.org>
Date: Fri Dec 09 23:24:20 2016

[tracing] Do not check enabled for filtering in TRACE_EVENT_CATEGORY_GROUP_ENABLED

The TRACE_EVENT_CATEGORY_GROUP_ENABLED sets enabled to true even when
filtering mode was turned on. This should be set only for recording and
etw modes.

BUG= 669611 , 670013

Review-Url: https://codereview.chromium.org/2547193002
Cr-Commit-Position: refs/heads/master@{#437686}

[modify] https://crrev.com/1ce3cd84139f6f60cb6a7458f0fbc95315241fd9/base/trace_event/trace_event.h

Th CL should fix the issue of some code doing:
TRACE_EVENT_CATEGORY_GROUP_ENABLED("disabled.xx")
if(enabled)
  do_lot_of_heavy_work();

But, we still have an issue of code doing:
TRACE_EVENT1(
    "disabled.xx", "", "snapshot", do_something_heavy());

I cannot think of a way to avoid the second one since we are doing the filtering much later after the enabled check in AddTraceEvent.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot