Re: [PATCH v5 2/6] perf record: Get the first sample time and last sample time

[Jin, Yao]

On 11/4/2017 6:24 PM, Jiri Olsa wrote:
> On Fri, Nov 03, 2017 at 01:29:42PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Oct 24, 2017 at 09:16:59AM +0200, Jiri Olsa escreveu:
> > > On Tue, Oct 24, 2017 at 10:03:05AM +0800, Jin, Yao wrote:
> > >
> > > SNIP
> > >
> > > > > hum, could you still unset the sample if there's no time given?
> > > > > and keep the speed in this case..
> > > > >
> > > > > jirka
> > > >
> > > > Hi Jiri,
> > > >
> > > > I check this question again. The '--time' option is for perf report but not
> > > > for perf record.
> > > >
> > > > For perf record, we have to always walk on all samples to get the time of
> > > > first sample and the time of last sample whatever buildid_all is enabled or
> > > > not enabled. So 'rec->tool.sample = NULL' is removed.
> > > >
> > > > Sorry, the previous mail was replied at midnight, I was drowsy. :(
> > > >
> > > > If my answer is correct, I will not send v6. If my understanding is still
> > > > not correct, please let me know.
> > >
> > > right, I did not realize we store this unconditionaly.. then yes, it's ok
> >
> > And should we store this unconditionally? What this patch is doing is
> > making 'perf record' unconditionally slower so that the generated
> > perf.data file becomes useful for some usecases, but not for all, only
> > people interested in using 'perf report/script --time' will benefit,
> > right?
>
> maybe we can also silently enable that when processing buildids,
> (which is set by default), there's no big performance hit once
> we already go through samples
>
> jirka

It's a good idea. Default enabling --timestamps in perf record since 
buildids is enabled by default as well.

But if buildids is not enabled, then it needs to check if --timestamps 
is enabled. I will follow this rule in v6.

Thanks
Jin Yao

> > I thought that we could get this sorted out in a different fashion, i.e.
> > getting the first timestamp is easy, even if we don't process build-ids,
> > right? To get the last one we could ask the kernel to insert an extra
> > dummy sample at the end, one that we know the size and thus can to to
> > the end of the file, rewind that size, get the event and parse the
> > sample, agreed?
> >
> > So I suggest that first make this conditional, i.e. 'perf record
> > --timestamps' will enable the logic you implemented, and as a followup,
> > if you agree, add the dummy, known size event at the end, and then even
> > when build-ids are not processed, the cost for getting the timestamps
> > will be next to zero.
> >
> > - Arnaldo
> >
> > - Arnaldo
> >   
> > > I think I've already acked this, anyway for the patchset:
> > >
> > > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > >
> > > thanks,
> > > jirka