|
|
Created:
4 years, 9 months ago by charliea (OOO until 10-5) Modified:
4 years, 8 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, darin-cc_chromium.org, jam, wfh+watch_chromium.org, nduca, Lei Zhang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontime: Add a static TimeTicks method that returns the underlying clock
In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.
BUG=597350
Committed: https://crrev.com/fdd7d49cc24ed079bfe164110970c25723d363a8
Cr-Commit-Position: refs/heads/master@{#384352}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Code review #Patch Set 3 : Changed to GetClock and an enum #
Total comments: 2
Patch Set 4 : Removed Clock::UNKNOWN #
Total comments: 3
Patch Set 5 : Use IsHighResolution #
Messages
Total messages: 51 (28 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== tracing: Adds the clock being used as metadata to the trace This CL is part of the broader "clock sync" architecture that we're trying to build for tracing. This architecture aims to allow us to combine multiple traces taken at the same time into a single trace by writing clock sync markers into each trace and aligning the traces based on those markers. Two traces are implicitly synced, though, if they both use timestamps from the same clock domain in the first place. In order for a consumer of the traces to determine this, though, we need to write some sort of a "clock domain identifier" into the trace. This CL does that. NOTE: Once this CL has been conceptually approved, I'll break it into two pieces: - Adding ClockId() to base::TimeTicks - Writing the clock domain metadata event into the trace DO NOT SUBMIT until this CL is broken apart. ========== to ========== tracing: Add the clock being used as metadata to the trace This CL is part of the broader "clock sync" architecture that we're trying to build for tracing. This architecture aims to allow us to combine multiple traces taken at the same time into a single trace by writing clock sync markers into each trace and aligning the traces based on those markers. Two traces are implicitly synced, though, if they both use timestamps from the same clock domain in the first place. In order for a consumer of the traces to determine this, though, we need to write some sort of a "clock domain identifier" into the trace. This CL does that. NOTE: Once this CL has been conceptually approved, I'll break it into two pieces: - Adding ClockId() to base::TimeTicks - Writing the clock domain metadata event into the trace DO NOT SUBMIT until this CL is broken apart. ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== tracing: Add the clock being used as metadata to the trace This CL is part of the broader "clock sync" architecture that we're trying to build for tracing. This architecture aims to allow us to combine multiple traces taken at the same time into a single trace by writing clock sync markers into each trace and aligning the traces based on those markers. Two traces are implicitly synced, though, if they both use timestamps from the same clock domain in the first place. In order for a consumer of the traces to determine this, though, we need to write some sort of a "clock domain identifier" into the trace. This CL does that. NOTE: Once this CL has been conceptually approved, I'll break it into two pieces: - Adding ClockId() to base::TimeTicks - Writing the clock domain metadata event into the trace DO NOT SUBMIT until this CL is broken apart. ========== to ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 ==========
Patchset #1 (id:80001) has been deleted
Description was changed from ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 ========== to ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 ==========
charliea@chromium.org changed reviewers: + nduca@chromium.org
charliea@chromium.org changed reviewers: + oysteine@google.com - nduca@chromium.org
oysteine@chromium.org changed reviewers: + oysteine@chromium.org
Usage lg2m from a tracing-owner perspective.
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824673002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824673002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
charliea@chromium.org changed reviewers: + thestig@chromium.org
thestig@, could you take a look for base/time ownership?
charliea@chromium.org changed reviewers: + danakj@chromium.org - thestig@chromium.org
Doh, looks like thestig@ is OOO. danakj@, do you think you'd be able to take a look?
https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco... base/time/time.h:686: static std::string ClockId(); it looks like this can return const char*. i kinda am wondering why strings and not an enum. but maybe that's good enough for you? it hides the platform-specific stuff in platform-specific files. but also makes it very hard to tell what this might return. https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#... base/time/time_win.cc:490: g_now_clock_id = now_clock_id; Why is this after the fence?
https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco... base/time/time.h:686: static std::string ClockId(); On 2016/03/26 00:40:33, danakj wrote: > it looks like this can return const char*. > > i kinda am wondering why strings and not an enum. but maybe that's good enough > for you? it hides the platform-specific stuff in platform-specific files. but > also makes it very hard to tell what this might return. Definitely agree that it makes it hard to see the possible return values. The main reason for the strings over the enums is that the primary way this will be used is as metadata for Chrome traces. The primary consumer of this (at least for right now) is going to be trace viewer (about:tracing). It's a whole separate thing from Chrome itself and pulls in traces from a variety of sources (Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine if Chrome was using the same clock as one of these other tracers that was tracing concurrently, allowing us to know if the two have timestamps on the same timeline. That being said, that's a lot of levels removed. Is there some idiomatic Chrome way to map enums to strings for printing? If we had that, returning an enum could be sufficient because we could just do something like: AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId())); https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#... base/time/time_win.cc:490: g_now_clock_id = now_clock_id; On 2016/03/26 00:40:33, danakj wrote: > Why is this after the fence? I'm not sure, but if the above comment is any indicator, it looks like it has to do with making sure g_qpc_ticks_per_second is changed before the function pointers. Naively, I'd expect that things should be okay as long as we change g_now_function and g_now_clock_id in the same block.
https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco... base/time/time.h:686: static std::string ClockId(); On 2016/03/28 15:40:51, charliea wrote: > On 2016/03/26 00:40:33, danakj wrote: > > it looks like this can return const char*. > > > > i kinda am wondering why strings and not an enum. but maybe that's good enough > > for you? it hides the platform-specific stuff in platform-specific files. but > > also makes it very hard to tell what this might return. > > Definitely agree that it makes it hard to see the possible return values. The > main reason for the strings over the enums is that the primary way this will be > used is as metadata for Chrome traces. The primary consumer of this (at least > for right now) is going to be trace viewer (about:tracing). It's a whole > separate thing from Chrome itself and pulls in traces from a variety of sources > (Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine if > Chrome was using the same clock as one of these other tracers that was tracing > concurrently, allowing us to know if the two have timestamps on the same > timeline. > > That being said, that's a lot of levels removed. Is there some idiomatic Chrome > way to map enums to strings for printing? https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... is a good example of doing that. > If we had that, returning an enum > could be sufficient because we could just do something like: > > AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId())); https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#... base/time/time_win.cc:490: g_now_clock_id = now_clock_id; On 2016/03/28 15:40:51, charliea wrote: > On 2016/03/26 00:40:33, danakj wrote: > > Why is this after the fence? > > I'm not sure, but if the above comment is any indicator, it looks like it has to > do with making sure g_qpc_ticks_per_second is changed before the function > pointers. Naively, I'd expect that things should be okay as long as we change > g_now_function and g_now_clock_id in the same block. Yeah, I stared at this for a while, and AFAICT the reason is because the now function can use g_qpc_ticks_p_s, so if the pointer is updated we want the g_qpc.. to be updated as well. However, the function does not use the clock id. The fence is magical, so putting things below makes them now depend on magic. So I'd prefer this was added above then. Maybe just above g_qpc_t_p_s?
https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco... base/time/time.h:686: static std::string ClockId(); On 2016/03/28 17:53:50, danakj wrote: > On 2016/03/28 15:40:51, charliea wrote: > > On 2016/03/26 00:40:33, danakj wrote: > > > it looks like this can return const char*. > > > > > > i kinda am wondering why strings and not an enum. but maybe that's good > enough > > > for you? it hides the platform-specific stuff in platform-specific files. > but > > > also makes it very hard to tell what this might return. > > > > Definitely agree that it makes it hard to see the possible return values. The > > main reason for the strings over the enums is that the primary way this will > be > > used is as metadata for Chrome traces. The primary consumer of this (at least > > for right now) is going to be trace viewer (about:tracing). It's a whole > > separate thing from Chrome itself and pulls in traces from a variety of > sources > > (Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine if > > Chrome was using the same clock as one of these other tracers that was tracing > > concurrently, allowing us to know if the two have timestamps on the same > > timeline. > > > > That being said, that's a lot of levels removed. Is there some idiomatic > Chrome > > way to map enums to strings for printing? > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... > is a good example of doing that. (And I think that such a function would belong in the tracing code, not in time.h) > > If we had that, returning an enum > > could be sufficient because we could just do something like: > > > > AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId())); >
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:220001) has been deleted
https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco... base/time/time.h:686: static std::string ClockId(); On 2016/03/28 17:54:21, danakj wrote: > On 2016/03/28 17:53:50, danakj wrote: > > On 2016/03/28 15:40:51, charliea wrote: > > > On 2016/03/26 00:40:33, danakj wrote: > > > > it looks like this can return const char*. > > > > > > > > i kinda am wondering why strings and not an enum. but maybe that's good > > enough > > > > for you? it hides the platform-specific stuff in platform-specific files. > > but > > > > also makes it very hard to tell what this might return. > > > > > > Definitely agree that it makes it hard to see the possible return values. > The > > > main reason for the strings over the enums is that the primary way this will > > be > > > used is as metadata for Chrome traces. The primary consumer of this (at > least > > > for right now) is going to be trace viewer (about:tracing). It's a whole > > > separate thing from Chrome itself and pulls in traces from a variety of > > sources > > > (Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine > if > > > Chrome was using the same clock as one of these other tracers that was > tracing > > > concurrently, allowing us to know if the two have timestamps on the same > > > timeline. > > > > > > That being said, that's a lot of levels removed. Is there some idiomatic > > Chrome > > > way to map enums to strings for printing? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched... > > is a good example of doing that. > > (And I think that such a function would belong in the tracing code, not in > time.h) > > > > If we had that, returning an enum > > > could be sufficient because we could just do something like: > > > > > > AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId())); > > > Done. https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#... base/time/time_win.cc:490: g_now_clock_id = now_clock_id; On 2016/03/28 17:53:50, danakj wrote: > On 2016/03/28 15:40:51, charliea wrote: > > On 2016/03/26 00:40:33, danakj wrote: > > > Why is this after the fence? > > > > I'm not sure, but if the above comment is any indicator, it looks like it has > to > > do with making sure g_qpc_ticks_per_second is changed before the function > > pointers. Naively, I'd expect that things should be okay as long as we change > > g_now_function and g_now_clock_id in the same block. > > Yeah, I stared at this for a while, and AFAICT the reason is because the now > function can use g_qpc_ticks_p_s, so if the pointer is updated we want the > g_qpc.. to be updated as well. > > However, the function does not use the clock id. The fence is magical, so > putting things below makes them now depend on magic. So I'd prefer this was > added above then. Maybe just above g_qpc_t_p_s? Done.
/bump :)
https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h#newco... base/time/time.h:647: UNKNOWN, Not sure that you really need the UNKNOWN here. It just makes you write it in switches and NOTREACHED() which is unneeded complexity. We never actually return that. I think you could let the global win one just default init to 0. LGTM otherwise.
https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h#newco... base/time/time.h:647: UNKNOWN, On 2016/03/30 19:51:58, danakj wrote: > Not sure that you really need the UNKNOWN here. It just makes you write it in > switches and NOTREACHED() which is unneeded complexity. We never actually return > that. I think you could let the global win one just default init to 0. > > LGTM otherwise. Done.
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1824673002/#ps260001 (title: "Removed Clock::UNKNOWN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824673002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824673002/260001
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc#... base/time/time_win.cc:521: TimeTicks::Clock TimeTicks::GetClock() { nit: I might be confused, but why not something like that instead: return IsHighResolution? WIN_QPC : WIN_ROLLOVER_PROTECTED_TIME_GET_TIME;
https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc#... base/time/time_win.cc:521: TimeTicks::Clock TimeTicks::GetClock() { On 2016/03/30 20:25:19, fmeawad wrote: > nit: I might be confused, but why not something like that instead: > return IsHighResolution? WIN_QPC : WIN_ROLLOVER_PROTECTED_TIME_GET_TIME; Oh, that's pretty nice yah.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc#... base/time/time_win.cc:521: TimeTicks::Clock TimeTicks::GetClock() { On 2016/03/30 20:26:29, danakj wrote: > On 2016/03/30 20:25:19, fmeawad wrote: > > nit: I might be confused, but why not something like that instead: > > return IsHighResolution? WIN_QPC : WIN_ROLLOVER_PROTECTED_TIME_GET_TIME; > > Oh, that's pretty nice yah. Agreed. That's a much nicer way of doing this. Done.
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1824673002/#ps280001 (title: "Use IsHighResolution")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824673002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824673002/280001
Message was sent while issue was closed.
Description was changed from ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 ========== to ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 ========== to ========== time: Add a static TimeTicks method that returns the underlying clock In a follow-up CL, the return value of this method will be written into traces recorded by Chrome so that, when syncing Chrome traces with external traces, we have more information to determine whether the two traces use timestamps from the same underlying clock and can therefore be combined. BUG=597350 Committed: https://crrev.com/fdd7d49cc24ed079bfe164110970c25723d363a8 Cr-Commit-Position: refs/heads/master@{#384352} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fdd7d49cc24ed079bfe164110970c25723d363a8 Cr-Commit-Position: refs/heads/master@{#384352} |