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

[Jin, Yao]

On 10/23/2017 11:04 PM, Jiri Olsa wrote:
> On Sat, Oct 21, 2017 at 07:27:50AM +0800, Jin Yao wrote:
> > In perf record, it's walked on all samples yet. So it's very easy to get
> > the first/last samples and save the time to perf file header via the
> > function write_sample_time().
> >
> > In later, perf report/script will fetch the time from perf file header.
> >
> > Change log:
> > -----------
> > v5: There is an issue that the sample walking can only work when
> >      '--buildid-all' is not enabled. So we need to let the walking
> >      be able to work even if '--buildid-all' is enabled and let the
> >      processing skips the dso hit marking for this case.
> >
> >      At first, I want to provide a new option "--record-time-boundaries".
> >      While after consideration, I think a new option is not very
> >      necessary.
> >
> > v3: Remove the definitions of first_sample_time and last_sample_time
> >      from struct record and directly save them in perf_evlist.
> >
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/builtin-record.c | 20 ++++++++++++--------
> >   1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index a6cbf16..bd711e8 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -391,8 +391,19 @@ static int process_sample_event(struct perf_tool *tool,
> >   {
> >   	struct record *rec = container_of(tool, struct record, tool);
> >   
> > -	rec->samples++;
> > +	if (rec->evlist->first_sample_time == 0)
> > +		rec->evlist->first_sample_time = sample->time;
> > +
> > +	rec->evlist->last_sample_time = sample->time;
> > +
> > +	/*
> > +	 * If --buildid-all is given, it marks all DSO regardless of hits,
> > +	 * so no need to process this sample.
> > +	 */
> > +	if (rec->buildid_all)
> > +		return 0;
> >   
> > +	rec->samples++;
> >   	return build_id__mark_dso_hit(tool, event, sample, evsel, machine);
> >   }
> >   
> > @@ -415,13 +426,6 @@ static int process_buildids(struct record *rec)
> >   	 */
> >   	symbol_conf.ignore_vmlinux_buildid = true;
> >   
> > -	/*
> > -	 * If --buildid-all is given, it marks all DSO regardless of hits,
> > -	 * so no need to process samples.
> > -	 */
> > -	if (rec->buildid_all)
> > -		rec->tool.sample = NULL;
>
> 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.

Thanks
Jin Yao