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

[Jin, Yao]

On 11/4/2017 12:29 AM, 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?

Yes, that makes sense.

> 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?

Yes, agree.

> 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.

So we will use 2 steps for the implementation.

Step1:
Upgrade this patch series to v6. The change is to add "--timestamps" 
option in perf record.

Step2:
As a followup patch series, let the kernel generate a dummy sample at 
the end. That maybe needs more discussions on what the format we should 
use in dummy sample and how to generate the dummy sample in kernel.

Let me complete the step1 first.

Thanks
Jin Yao

> - Arnaldo
>
> - Arnaldo
>   
> > I think I've already acked this, anyway for the patchset:
> >
> > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> >
> > thanks,
> > jirka