Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162183002:
Binds an ID to a Globally Unique ID. (Closed)
|
Created:
4 years, 5 months ago by chiniforooshan Modified:
4 years, 4 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@netinst_macros Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBinds an ID to a Globally Unique ID.
Similar to https://codereview.chromium.org/2142023003/, but instead of
binding two IDs from the same process this macro adds a bind_global =
true field to arguments telling trace importers that the first ID from
the current process is identical to the second ID from "any" process.
BUG=catapult:#2465
Patch Set 1 #Patch Set 2 : syned #Patch Set 3 : synced with origin/master #Patch Set 4 : reimplemented #
Total comments: 4
Messages
Total messages: 17 (4 generated)
Description was changed from ========== Binds an ID to a Globally Unique ID. Similar to https://codereview.chromium.org/2142023003/, but instead of binding two IDs from the same process this macro adds a bind_global = true field to arguments telling trace importers that the first ID from the current process is identical to the second ID from "any" process. BUG=catapult:#2465 ========== to ========== Binds an ID to a Globally Unique ID. Similar to https://codereview.chromium.org/2142023003/, but instead of binding two IDs from the same process this macro adds a bind_global = true field to arguments telling trace importers that the first ID from the current process is identical to the second ID from "any" process. BUG=catapult:#2465 ==========
chiniforooshan@chromium.org changed reviewers: + caseq@chromium.org, dproy@chromium.org, primiano@chromium.org
Andrey, do you have any concerns about linking to a GUIDs, for cross process linkings? GUIDs can be created by including tracing_process_ids in scope strings?
On 2016/07/20 20:22:14, chiniforooshan wrote: > Andrey, do you have any concerns about linking to a GUIDs, for cross process > linkings? GUIDs can be created by including tracing_process_ids in scope > strings? The question is, would we have to send yet another id (GUID in this case) along with the ResourceRequest? I would like to avoid that and re-use some value that we already share between the browser and the child -- either the pid if we agree that it is safe to do so, or the tracing_process_id that memory instrumentation uses.
On 2016/07/22 02:32:29, caseq wrote: > On 2016/07/20 20:22:14, chiniforooshan wrote: > > Andrey, do you have any concerns about linking to a GUIDs, for cross process > > linkings? GUIDs can be created by including tracing_process_ids in scope > > strings? > > The question is, would we have to send yet another id (GUID in this case) along > with the ResourceRequest? I would like to avoid that and re-use some value that > we already share between the browser and the child -- either the pid if we agree > that it is safe to do so, or the tracing_process_id that memory instrumentation > uses. No, we don't have to send another ID. The idea is to use the same resource ID that already is sent and make it unique by adding the tracing_process_id, which is known to both sides, to its scope string.
On 2016/07/22 14:31:29, chiniforooshan wrote: > On 2016/07/22 02:32:29, caseq wrote: > > On 2016/07/20 20:22:14, chiniforooshan wrote: > > > Andrey, do you have any concerns about linking to a GUIDs, for cross process > > > linkings? GUIDs can be created by including tracing_process_ids in scope > > > strings? > > > > The question is, would we have to send yet another id (GUID in this case) > along > > with the ResourceRequest? I would like to avoid that and re-use some value > that > > we already share between the browser and the child -- either the pid if we > agree > > that it is safe to do so, or the tracing_process_id that memory > instrumentation > > uses. > > No, we don't have to send another ID. The idea is to use the same resource ID > that already is sent and make it unique by adding the tracing_process_id, which > is known to both sides, to its scope string. We currently expect scope string to be statically allocated, don't we? So I assume this might increase complexity a bit. How about we support composite IDs instead of abusing scope string -- but just for binding? Basically, if you use in a regular TRACE_EVENT_ASYNC* (or FLOW), you must use simple id, but just for binding you can use composite ids that would be passed as traced values?
On 2016/07/25 16:18:17, caseq wrote: > On 2016/07/22 14:31:29, chiniforooshan wrote: > > On 2016/07/22 02:32:29, caseq wrote: > > > On 2016/07/20 20:22:14, chiniforooshan wrote: > > > > Andrey, do you have any concerns about linking to a GUIDs, for cross > process > > > > linkings? GUIDs can be created by including tracing_process_ids in scope > > > > strings? > > > > > > The question is, would we have to send yet another id (GUID in this case) > > along > > > with the ResourceRequest? I would like to avoid that and re-use some value > > that > > > we already share between the browser and the child -- either the pid if we > > agree > > > that it is safe to do so, or the tracing_process_id that memory > > instrumentation > > > uses. > > > > No, we don't have to send another ID. The idea is to use the same resource ID > > that already is sent and make it unique by adding the tracing_process_id, > which > > is known to both sides, to its scope string. > > We currently expect scope string to be statically allocated, don't we? So I > assume this might increase complexity a bit. > How about we support composite IDs instead of abusing scope string -- but just > for binding? Basically, if you use in a regular TRACE_EVENT_ASYNC* (or FLOW), > you must use simple id, but just for binding you can use composite ids that > would be passed as traced values? Re-implemented the macros after our offline discussion. PTAL. Example usages of the new macros can be seen in trace_event_unittest.cc. Note that composite IDs are only supported in LINK_ID macros and only as the second arguments. If they are used in other places, as trace IDs, the prefix will be ignored and only the last part will be used. This will be fixed in tracing V2 (supporting it in V1 needs adding new fields to the TraceEvent structure).
primiano@chromium.org changed reviewers: + oysteine@chromium.org
First of all apologies for the delay on this. Next time ping me over IM if you see that I don't give heartbeats in 24h. I think I lost track of this while I was OOO on Tue. In all honesty I am not terribly excited by this. Very likely this is due to the fact that the current ID system is already all hairy, but in all honesty this CL gives me the feeling of adding yet another layer on top of all the mess. Definitely the lack of a proper description of what's going on in the commit message and the lack of a comment on that |prefix| argument don't help. One more sure comment is that the change in trace_event.h seems not consistent with the rest of the code (using directly TracedValue there). I'm going to be away for two weeks, so I suggest you follow up this with some other tracing owner. A good rule of thumb to check the API is the following: take some experienced tracing user and show them the new ID system. Do they understand the new API mechanism being introduced? If the answer is "hmm" it's the sign that something needs to be revisited. skyostil@ is the first name that comes on top of my mind to get good feedback for tracing API. Sorry to drop the ball on this right now, just my 2 weeks OOO is not playing great. https://codereview.chromium.org/2162183002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2162183002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:41: // DEPRECATED: do not use. Consider using (GLOBAL_)TRACE_ID(_WITH_SCOPE) macros, so the doubt that I have when I see that both these are deprecated is: what is the default behavior if somebody creates an event with just an id? will it still be mangled? what will be its visibility? This is still not clear to me after this CL. https://codereview.chromium.org/2162183002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:48: #define GLOBAL_TRACE_ID(...) \ I thought that the ID is already global, reason for which we have TRACE_ID_MANGLE. What is the difference between non-global and GLOBAL_TRACE_ID then? My concern is that we still make sure that whatever new semantic we introduce has to be not ambiguous with the previous traces. In other words, whatever new we introduce in the importer, it has to still import correctly the older traces. https://codereview.chromium.org/2162183002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:374: std::unique_ptr<TracedValue> value(new TracedValue()); \ I don't see any precedent of using TracedValue directly inside here. +oysteine is definitely a better person to figure out the layering here, but I think there is a reason (I think related with subprojects) why everything here goes via trace_event_internal. https://codereview.chromium.org/2162183002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:462: WithScope(const char* scope, bool is_global, unsigned long long prefix, what is this prefix? I don't see documented this anyways. While looking to this API as a client the only thing I see is just "yet another long long argument"
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Here's a naive question -- I didn't see it in the design docs but may have missed it: is there an advantage to linked ids vs. dumping an object snapshot with a "parent" pointer or something that ties the object to another? At least that works for frameblaming. I'm also wondering how linked ids work when an object gets destroyed. Do we need to unlink them somehow?
I think this discussion is going to be simpler if we break the CL apart. The local ID support is a good candidate to be extracted. I think our current way of supporting process-local id (i.e. mangling these) is broken because it does not give us a way to refer to such id outside of its process (this is essentially what we need for Network). Let's fix it by explicitly supporting per-process ids. Since these are presently global, let's make that the default and require some explicit way of specifying locals (either as a special flag for TraceID::WithScope or some TraceID nested class along ForceMangle/DontMangle). We could easily retain the backward compatibility by emitting both mangled id and localId at the trace generation time, but I don't think we have to -- just use a field name different from id for local ids. Primiano and Sami, does this make sense to you? WRT using TracedValue -- I agree this is a layering violation. Can we get away with just using arguments for linking?
On 2016/08/05 18:14:58, Sami wrote: > Here's a naive question -- I didn't see it in the design docs but may have > missed it: is there an advantage to linked ids vs. dumping an object snapshot > with a "parent" pointer or something that ties the object to another? At least > that works for frameblaming. I think what you describe is semantically close to what Ehsan has implemented. However, rather than doing object dumps on the call sites, we'd like this to be available in general form for all types of events that may use id and handled transparently on the trace consumer side (i.e. just assume linked ids are equal when connecting async slices). > I'm also wondering how linked ids work when an object gets destroyed. Do we need > to unlink them somehow? Definitely -- some of the ids that we use may be re-used, so we need to manage the life time of the links. One option here is doing it explicitly, another is perhaps discard the association when an async event is terminated (should apparently work for network, but slightly more fragile in general).
On 2016/08/08 23:27:32, caseq wrote: > I think this discussion is going to be simpler if we break the CL apart. Always a good idea :) > The local ID support is a good candidate to be extracted. I think our current > way of supporting process-local id (i.e. mangling these) is broken because it > does not give us a way to refer to such id outside of its process (this is > essentially what we need for Network). > > Let's fix it by explicitly supporting per-process ids. Since these are presently > global, let's make that the default and require some explicit way of specifying > locals (either as a special flag for TraceID::WithScope or some TraceID nested > class along ForceMangle/DontMangle). We could easily retain the backward > compatibility by emitting both mangled id and localId at the trace generation > time, but I don't think we have to -- just use a field name different from id > for local ids. > Primiano and Sami, does this make sense to you? Agreed that referring to ids from another process would be useful. (Although don't we already do something like that with pidRef?) Since all the ids are effectively global right now, I'm wondering whether we even need local ids for anything? If there's a risk of collisions you can always set a different scope. It might simplify our lives if we only needed to support one scheme. > I think what you describe is semantically close to what Ehsan has implemented. > However, rather than doing object dumps on the call sites, we'd like this to be > available in general form for all types of events that may use id and handled > transparently on the trace consumer side (i.e. just assume linked ids are equal > when connecting async slices). Sure that makes sense. I'm just wondering if once we add all the necessary features like lifetime management, we'll have replicated the object snapshot code. (Happy to chat about this over vc if you guys want.)
OK, I'll break this into 2 CLs. But, can we please try to converge on some design issues here before I start the re-implementation? 1- As far as I remember, currently some IDs are process-local, e.g. objects and contexts, and some IDs are global, e.g. async events. There is no default behaviour. So, to be able to import old trace files, WDYT about deciding the process-locality of an ID according to the value of the "id_is_global" field, if it is present, and otherwise, if the field is not present, according to the event phase (i.e. process-local if the event is an object or context event, and global if the event is an async event)? 2- As for not having local IDs at all and avoiding collisions by setting different scope, we have to include a PID-like number int the scope for that. That A. looks a little bit hacky, B. means string manipulation for every single trace event that has a local ID and C. whenever I talk about using PIDs I get a lot of resistance. So, I think having a flag to show whether the ID is process-local or not is easier. 3- As for life of linked IDs, I think it makes sense to keep IDs linked only as long as both IDs are live. This means discarding the object ID links when the object is deleted and discarding async event ID links when the event is ended. Can you think of a scenario where this will break things? 4- As for using TraceValues in the macro, can someone explain why it breaks the layering and what is the recommended way around it? Using arguments doesn't work here since we have more than 2.
On 2016/08/09 10:15:16, Sami wrote: > Agreed that referring to ids from another process would be useful. (Although > don't we already do something like that with pidRef?) pidRef is what the frame blamer uses, right? Well, again, the goal is to make it generally available on lower level since this would be useful for several different applications, network requests being one of them. > Since all the ids are effectively global right now, I'm wondering whether we > even need local ids for anything? Anything that originates on the renderer side, I guess. Network requests are one example, compositor frames may be another. > If there's a risk of collisions you can always set a different scope. I think this is what Ehsan originally proposed. If this refers to the scopes that we support in TRACE_ID_WITH_SCOPE(), those are static strings presently (i.e. we don't manage ownership there) -- using pids there would require supporting dynamically allocated scope strings. If we go for an approach that requires us keeping the ownership of a scoped id, we could as well jut for for composite ids I think -- basically, allow an id to consist of several components, scope being one of them and pid being another.
Adding one more random thought here: it'd be great if whatever we come up with also does something sensible in --single-process mode. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
