This is an archive of the discontinued LLVM Phabricator instance.

Make LLVM timer reprintable: that is, make more than one print action on the same timer feasible
ClosedPublic

Authored by george.karpenkov on Feb 9 2018, 11:14 AM.

Details

Summary

Currently, each LLVM timer can be only printed once, as the act of printing clear the timer.
I don't think this is always a good idea, as printing twice might be a requirement (e.g. dumping statistics in a serialized form as well as printing it to stdout, or dumping statistics in multiple places).
In the analyzer we are actually hitting those limitations (e.g. it would be nice to have statistics in the produced HTML report as well as on the command line).

Moreover, the current printing mechanism implicitly assumes that the timer is stopped -- and prints zero otherwise.
This patch relaxes this assumption (as I don't think printing a running timer should be a giant problem or should crash), but if it's an issue I could change it to make the assumption explicit by asserting !isRunning.

Diff Detail

Event Timeline

MatzeB accepted this revision.Feb 9 2018, 4:34 PM

I agree that a function named printXXX() should not also clear the timer.
As far as I understand it this shouldn't change any behavior in the existing clang/llvm code, so LGTM.

This revision is now accepted and ready to land.Feb 9 2018, 4:34 PM
This revision was automatically updated to reflect the committed changes.

Unfortunately this change is causing trouble over in swift where we _did_ rely on the fact that printing timers cleared them. In particular: we wanted to control printing the timers as json to our own chosen stats stream on shutdown (not the llvm one) and once printed, we did not expect them to print restart _again_ to stderr when llvm shuts down.

Is there some way we can either revert this change, or add a method that explicitly clears all timers?

@graydon could you just explicitly .clear() them?

@graydon could you just explicitly .clear() them?

There is no public API with which to do so (or indeed, to access them at all). I would be happy to otherwise. Shall I add such an API?

@graydon Ah right, .clear() is public on a Timer, but not exposed through TimerGroup.
I'm fine with adding such an API.
I'm not sure whether anyone else would object ( @MatzeB @ab @thegameg ?)

I'm not sure whether anyone else would object ( @MatzeB @ab @thegameg ?)

Sounds good to me.