Page MenuHomePhabricator

Time profiler: small fixes and optimizations
ClosedPublic

Authored by anton-afanasyev on Apr 14 2019, 4:35 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 14 2019, 4:35 AM

Looks good ignoring the json bits.

Re license:

https://reviews.llvm.org/D58675
This is the first part of time tracing system, I have splitted them cause this part is mostly written by Aras Pranckevicius except of several minor fixes concerning formatting.

So i can't and won't claim any legal knowledge, but it maybe would be good for him to at least comment here, that he is ok with this?

llvm/lib/Support/TimeProfiler.cpp
20 ↗(On Diff #195054)

Unused header?

37–38 ↗(On Diff #195054)

Ah yes, they are allocated when created. Hmm.

47–48 ↗(On Diff #195054)

Does

Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail());

not work?
(also, the std::string returned from Detail function invocation is moved?)

57 ↗(On Diff #195054)

I feel like this should be

static cl::opt<unsigned> TimeProfileGranularity(
    "time-profile-granularity",
    cl::desc("<fixme>"),
    cl::init(500));

?

65 ↗(On Diff #195054)

Yes, good point.

anton-afanasyev marked 7 inline comments as done.Apr 14 2019, 8:25 AM
anton-afanasyev added a subscriber: aras-p.

Looks good ignoring the json bits.

Re license:

https://reviews.llvm.org/D58675
This is the first part of time tracing system, I have splitted them cause this part is mostly written by Aras Pranckevicius except of several minor fixes concerning formatting.

So i can't and won't claim any legal knowledge, but it maybe would be good for him to at least comment here, that he is ok with this?

Oh, sure! @aras-p -- I have changed license header to fit it with current LLVM Relicensing state. Are you ok with this? (see https://reviews.llvm.org/D60663#change-jr7Lagn5WNFy)

llvm/lib/Support/TimeProfiler.cpp
20 ↗(On Diff #195054)

Yes, thanks.

47–48 ↗(On Diff #195054)

Ok, changed.

57 ↗(On Diff #195054)

I planned to change this later together with unit tests (cause they need small time granularity), but can fix it now. Changed, thanks!

anton-afanasyev marked 2 inline comments as done.

Updated

Looks good ignoring the json bits.

Re license:

https://reviews.llvm.org/D58675
This is the first part of time tracing system, I have splitted them cause this part is mostly written by Aras Pranckevicius except of several minor fixes concerning formatting.

So i can't and won't claim any legal knowledge, but it maybe would be good for him to at least comment here, that he is ok with this?

I've communicated to Aras by mail, he is ok with new license header:

"Sure! I started my branch before the LLVM license change. The new one is fine."

lebedev.ri accepted this revision.Apr 15 2019, 12:14 PM

Ok, LG, thank you!

llvm/lib/Support/TimeProfiler.cpp
30 ↗(On Diff #195069)

microsecon*d*s

70–71 ↗(On Diff #195069)

I *think* && are not needed.

92 ↗(On Diff #195069)

This will only track events that are longer than TimeTraceGranularity, which i think conflicts
with the "Minimum time granularity (in microseconds) traced by time profiler"
So either >= or reword the description a bit?

156–157 ↗(On Diff #195069)

Since TimeTraceProfiler will be always heap-allocated (so stack usage is not a concern)
i think this is safe, and maybe even bump it a bit, depending on the values you see in average profiles?

This revision is now accepted and ready to land.Apr 15 2019, 12:14 PM

So i can't and won't claim any legal knowledge, but it maybe would be good for him to at least comment here, that he is ok with this?

Yes, absolutely fine. The only reason why some files started with the old license blurb is because I started the branch before licensing switch kicked in.

This revision was automatically updated to reflect the committed changes.
sammccall added inline comments.
llvm/trunk/lib/Support/TimeProfiler.cpp
27

I know this is late, but... this shows up in the help for any tool that links in libSupport, many of which don't support the time profiler. Can you mark this as hidden or (preferably) move this to cc1_main?

anton-afanasyev marked 2 inline comments as done.Jul 24 2019, 5:16 AM
anton-afanasyev added inline comments.
llvm/trunk/lib/Support/TimeProfiler.cpp
27

@sammccall Yes, thanks! Here is the fix https://reviews.llvm.org/D65202 , please, review it.

sammccall added inline comments.Jul 24 2019, 5:28 AM
llvm/trunk/lib/Support/TimeProfiler.cpp
27

Thanks! A few nits but that's a big improvement I think.