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

[Jiri Olsa <jolsa@redhat.com>]

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

> 
> 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@kernel.org>
> > 
> > thanks,
> > jirka