Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381083003:
The TRACE_LINK_IDS macro (Closed)
|
Created:
4 years, 2 months ago by chiniforooshan Modified:
4 years, 2 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, dproy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
Committed: https://crrev.com/ecfbdf571cc36bc1f9282032a27177d482202580
Cr-Commit-Position: refs/heads/master@{#425069}
Patch Set 1 #
Total comments: 11
Patch Set 2 : comments #Patch Set 3 : TracedValue -> CTTF #
Total comments: 4
Patch Set 4 : comments #
Total comments: 1
Patch Set 5 : comments #Patch Set 6 : fix the debug build #Patch Set 7 : . #
Messages
Total messages: 40 (21 generated)
The CQ bit was checked by chiniforooshan@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...
chiniforooshan@chromium.org changed reviewers: + caseq@chromium.org, primiano@chromium.org
PTAL
Description was changed from
==========
The TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
==========
to
==========
The TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
==========
chiniforooshan@chromium.org changed reviewers: + tracing-reviews@chromium.org
chiniforooshan@chromium.org changed reviewers: + tracing+reviews@chromium.org - tracing-reviews@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comment https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:579: std::unique_ptr<base::trace_event::TracedValue> dict_value() const { Let's move it out-of-line, it's too big.
primiano@chromium.org changed reviewers: + oysteine@chromium.org
I suggest sanity check this with oysteine@ who knows all the quirks about these includes. https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:18: #include "base/format_macros.h" remember to remove the unnecessary .h once you move the method out-of-line https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:24: #include "base/trace_event/trace_event_argument.h" Yeah please don't pull extra includes in this file or will very likely create a mess in blink. https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:395: "linked_id", target_id.dict_value()); \ don't you need a std::move here? https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:579: std::unique_ptr<base::trace_event::TracedValue> dict_value() const { On 2016/10/01 01:33:53, caseq wrote: > Let's move it out-of-line, it's too big. +1 this is definitely beyon the limits for an inline function (see https://google.github.io/styleguide/cppguide.html#Inline_Functions) Also call it AsTracedValue() for uniformity with the other code. also maybe the signature here schould be unique_ptr<ConvertableToTraceFormat> TracedValue is not exposed by this header. I wonder if doing that would cause problems in v8 or blink https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:581: new base::trace_event::TracedValue()); makeunique
chiniforooshan@chromium.org changed reviewers: + thakis@chromium.org - tracing+reviews@chromium.org
+Nico for base/BUILD.gn +Oystein: Could you please sanity check this change? Thanks a lot! https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:18: #include "base/format_macros.h" On 2016/10/01 01:50:00, Primiano Tucci wrote: > remember to remove the unnecessary .h once you move the method out-of-line Done. https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:24: #include "base/trace_event/trace_event_argument.h" On 2016/10/01 01:50:00, Primiano Tucci wrote: > Yeah please don't pull extra includes in this file or will very likely create a > mess in blink. Done. https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:395: "linked_id", target_id.dict_value()); \ On 2016/10/01 01:50:00, Primiano Tucci wrote: > don't you need a std::move here? It's a return value. If I actually do std::move here I get a warning: "moving a temporary object prevents copy elision". This is similar to many other cases already in the code. e.g. we don't std::move here: https://cs.chromium.org/chromium/src/cc/debug/benchmark_instrumentation.cc?l=18. https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:579: std::unique_ptr<base::trace_event::TracedValue> dict_value() const { On 2016/10/01 01:50:00, Primiano Tucci wrote: > On 2016/10/01 01:33:53, caseq wrote: > > Let's move it out-of-line, it's too big. > > +1 this is definitely beyon the limits for an inline function (see > https://google.github.io/styleguide/cppguide.html#Inline_Functions) > > Also call it AsTracedValue() for uniformity with the other code. > > also maybe the signature here schould be unique_ptr<ConvertableToTraceFormat> > > TracedValue is not exposed by this header. I wonder if doing that would cause > problems in v8 or blink Done. https://codereview.chromium.org/2381083003/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:581: new base::trace_event::TracedValue()); On 2016/10/01 01:50:00, Primiano Tucci wrote: > makeunique Done.
base/BUILD.gn lgtm
On 2016/10/04 15:39:08, chiniforooshan wrote: ping
LGTM with some comments I think you need to update also tools/gn/bootstrap/bootstrap.py Please can you update the Trace Event Format doc [1] to reflect the state of things? At this point I have absolutely no clue about how id, bind_id, scope and link_id work any more. I trust that there is a rationale at the end, and historical reasons why we ended up in this state, just please make it explicit. [1] https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... https://codereview.chromium.org/2381083003/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2381083003/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:577: dict_value() const; lowe_case is only for inline methods. s/dict_value/AsConvertableToTraceFormat/ as that is what TraceConfig also uses https://codereview.chromium.org/2381083003/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:1133: class ConvertableToTraceFormat; shouldn't this fwd declaration be *before* you use it on line 576?
primiano@chromium.org changed reviewers: + fmeawad@chromium.org
Ah, also +fadi for common/trace_event_common.h to make sure this isn't somehow breaking tracing in v8.
https://codereview.chromium.org/2381083003/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2381083003/diff/60001/base/BUILD.gn#newcode941 base/BUILD.gn:941: "trace_event/trace_event.cc", Why not add the function to trace_event_impl.cc? (I know the _impl is a bit of a misnomer and should probably be renamed, but I don't think we need two separate .cc's here).
On 2016/10/06 18:40:43, oystein wrote: > https://codereview.chromium.org/2381083003/diff/60001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/2381083003/diff/60001/base/BUILD.gn#newcode941 > base/BUILD.gn:941: "trace_event/trace_event.cc", > Why not add the function to trace_event_impl.cc? (I know the _impl is a bit of a > misnomer and should probably be renamed, but I don't think we need two separate > .cc's here). (That said adding a trace_id.h/.cc would probably be a good thing, but I don't think we should have both trace_event_impl.cc and trace_event.cc).
primiano: (changing the public Chrome tracing doc) will update the doc before
landing this CL.
oystein: as discussed offline, factoring out this code to trace_id.{h, cc}
requires making all other inline methods out-of-line, since trace_id.h cannot
include trace_event_common.h but the methods use constants from
trace_event_common.h. So, I moved the implementation to trace_event_impl.h.
Thanks everyone for reviewing!
https://codereview.chromium.org/2381083003/diff/40001/base/trace_event/trace_...
File base/trace_event/trace_event.h (right):
https://codereview.chromium.org/2381083003/diff/40001/base/trace_event/trace_...
base/trace_event/trace_event.h:577: dict_value() const;
On 2016/10/06 16:28:27, Primiano Tucci - slow til 10th wrote:
> lowe_case is only for inline methods.
> s/dict_value/AsConvertableToTraceFormat/ as that is what TraceConfig also uses
Done.
https://codereview.chromium.org/2381083003/diff/40001/base/trace_event/trace_...
base/trace_event/trace_event.h:1133: class ConvertableToTraceFormat;
On 2016/10/06 16:28:26, Primiano Tucci - slow til 10th wrote:
> shouldn't this fwd declaration be *before* you use it on line 576?
Done.
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, primiano@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2381083003/#ps80001 (title: "comments")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, caseq@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2381083003/#ps100001 (title: "fix the debug build")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chiniforooshan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, caseq@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2381083003/#ps120001 (title: ".")
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
==========
The TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
==========
to
==========
The TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
The TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
==========
to
==========
The TRACE_LINK_IDS macro
We would like to be able to make two event IDs identical; so,
e.g. we can do something like the following:
TRACE_EVENT_ASYNC_BEGIN0("cat", "an_async_event", "0x1000");
TRACE_EVENT_LINK_IDS("cat", "a_link_event", "0x1000", "0x2000");
TRACE_EVENT_ASYNC_END("cat", "an_async_event", "0x2000");
This was done in https://codereview.chromium.org/2142023003. But,
in that patch, I misused the "bind_id" field in the
implementation of the macro to avoid introducing a new field.
That field overwriting caused some confusions because "bind_id"
was introduced for flow events.
This patch fixes the "bind_id" misusage. Also, it makes sure that
the TRACE_ID_LOCAL and TRACE_ID_GLOBAL macros that were
introduced in https://codereview.chromium.org/2253973003 work
properly when nested inside the TRACE_LINK_IDS macro.
For more context about why the TRACE_LINK_IDS was introduced in
the first place:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s
BUG=catapult:#2465
Committed: https://crrev.com/ecfbdf571cc36bc1f9282032a27177d482202580
Cr-Commit-Position: refs/heads/master@{#425069}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ecfbdf571cc36bc1f9282032a27177d482202580 Cr-Commit-Position: refs/heads/master@{#425069} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
