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

["Jin, Yao" <yao.jin@linux.intel.com>]

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