[vsipl++] [patch] Profile_event class

[Jules Bergmann]

Don McCoy wrote:
 > Thank you Jules for the suggestions and the detailed explanations as
 > well.  All have been addressed with this revision.  The important points
 > being:
 >
 >   - impl_performance() works independently of the profiler being enabled
 >   - It does share a timer resource internally for now
 >   - FFT/FFTM size is now part of the descriptive tag in the profile 
output.
 >   - Tags now use S,D,C,Z for float, double, complex<float> and
 > complex<double> respectively.
 >   - The tags are now generated at one point - from the base class
 > (rather than in each derived Fft type).

Don,

Sorry that I did not get to this sooner.

This looks good.  I have 4 comments below, but once you're happy with
those, please check it in.

			thanks,
			-- Jules

 >
 > Also, thanks to Stefan for helping me find a defect in my handling of
 > the timer resource.  This was related to making an unintentional copy of
 > the object owning the timer.  I mention this because it helped me also
 > to realize that when an Fft object is copied a new timer is created.
 > This effectively erases the history that impl_performance() is able to
 > report.  I don't think this is unreasonable behavior, but thought I'd
 > point it out.

I agree.

But how does that happen though?  Looking at the Profile_event class,
it uses the default copy constructor, so copying a Profile_event
should copy the history (but produce a unique object so that
subsequent changes to either will cause them to diverge).

				-- Jules

 > @@ -220,7 +221,7 @@
 >      {
 >        float mflops = (*cur).second.count * (*cur).second.value /
 >          (1e6 * TP::seconds((*cur).second.total));
 > -      file << (*cur).first
 > +      file << (*cur).first.c_str()

[1] You should be able to pass a std::string directly to the stream.

 >             << delim << TP::ticks((*cur).second.total)
 >             << delim << (*cur).second.count
 >             << delim << (*cur).second.value


 > +
 > +// Create a readable tag from parameters.
 > +template <int D>
 > +struct desc_dim { static char * value() { return "FFT "; } };
 > +template <>
 > +struct desc_dim<2> { static char * value() { return "FFTM "; } };

[2] I think it would be better to match the class name here.

It's too bad we had to change the class names to Fft and Fftm.


 > +class Profile_event
 > +{
 > +  typedef DefaultTime    TP;
 > +
 > +public:
 > +  Profile_event(std::string name, unsigned int ops_count = 0)
 > +    : name_(name), ops_(ops_count)
 > +  {}
 > +
 > +  ~Profile_event() {}

[3] Can you put a comment saying that the default copy constructor and
assignment operator are OK?

 > +};


 > +class Scope_profile_event

[4] To be safe, I think this class should be made Non_copyable.

 > +{
 > +public:
 > +  Scope_profile_event(Profile_event& event)
 > +    : id_(prof->event(event.name(), event.ops(), 0, event.start())),
 > +      event_(event)
 > +  {}
 > +
 > +  ~Scope_profile_event() { prof->event(event_.name(), 0, id_, 
event_.stop()); }
 > +
 > +private:
 > +  int id_;
 > +  Profile_event& event_;
 > +};
 > +


-- 
Jules Bergmann
CodeSourcery
jules at codesourcery.com
(650) 331-3385 x705