Messages in this thread |  | | Subject | Re: [PATCH v5 2/6] perf record: Get the first sample time and last sample time | From | "Jin, Yao" <> | Date | Mon, 6 Nov 2017 08:28:16 +0800 |
| |
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
|  |