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

[Jin, Yao]

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 

thanks,
jirka

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

[Jin, Yao]

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 

thanks,
jirka

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

[Jiri Olsa]

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 
> > 
> > thanks,
> > jirka

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

[Jiri Olsa]

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 
> > 
> > thanks,
> > jirka

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 

thanks,
jirka

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 

thanks,
jirka

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

[Arnaldo Carvalho de Melo]

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?

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 
> 
> thanks,
> jirka

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

[Arnaldo Carvalho de Melo]

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?

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 
> 
> thanks,
> jirka

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

[Jin, Yao]

On 10/24/2017 3:16 PM, Jiri Olsa wrote:

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

I think I've already acked this, anyway for the patchset:

Acked-by: Jiri Olsa 

thanks,
jirka



Hi Arnaldo,

Is this patch-set OK for merging or anything I should improve?

Thanks
Jin Yao

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

[Jin, Yao]

On 10/24/2017 3:16 PM, Jiri Olsa wrote:

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

I think I've already acked this, anyway for the patchset:

Acked-by: Jiri Olsa 

thanks,
jirka



Hi Arnaldo,

Is this patch-set OK for merging or anything I should improve?

Thanks
Jin Yao

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

[Jin, Yao]

On 10/24/2017 3:16 PM, Jiri Olsa wrote:

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

I think I've already acked this, anyway for the patchset:

Acked-by: Jiri Olsa 

thanks,
jirka



Hi Jiri,

Thanks a lot for your review comments and your helps!

Thanks
Jin Yao

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

[Jin, Yao]

On 10/24/2017 3:16 PM, Jiri Olsa wrote:

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

I think I've already acked this, anyway for the patchset:

Acked-by: Jiri Olsa 

thanks,
jirka



Hi Jiri,

Thanks a lot for your review comments and your helps!

Thanks
Jin Yao

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

[Jiri Olsa]

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

I think I've already acked this, anyway for the patchset:

Acked-by: Jiri Olsa 

thanks,
jirka

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

[Jiri Olsa]

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

I think I've already acked this, anyway for the patchset:

Acked-by: Jiri Olsa 

thanks,
jirka

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

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

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

[Arnaldo Carvalho de Melo]

Em Tue, Oct 24, 2017 at 01:38:26AM +0800, Jin, Yao escreveu:
> 
> 
> 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 
> > > ---
> > >   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
> > 
> 
> Oh, yes, I should do that. Thanks Jiri!
> 
> Hi Arnaldo, do you have any comment for v5?

I'll try to look at it, but don't that hold v6, I'd say :-\

- Arnaldo
 
> I will prepare v6 for adding fixes according to all comments.
> 
> Thanks
> Jin Yao

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

[Arnaldo Carvalho de Melo]

Em Tue, Oct 24, 2017 at 01:38:26AM +0800, Jin, Yao escreveu:
> 
> 
> 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 
> > > ---
> > >   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
> > 
> 
> Oh, yes, I should do that. Thanks Jiri!
> 
> Hi Arnaldo, do you have any comment for v5?

I'll try to look at it, but don't that hold v6, I'd say :-\

- Arnaldo
 
> I will prepare v6 for adding fixes according to all comments.
> 
> Thanks
> Jin Yao

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



Oh, yes, I should do that. Thanks Jiri!

Hi Arnaldo, do you have any comment for v5?

I will prepare v6 for adding fixes according to all comments.

Thanks
Jin Yao

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



Oh, yes, I should do that. Thanks Jiri!

Hi Arnaldo, do you have any comment for v5?

I will prepare v6 for adding fixes according to all comments.

Thanks
Jin Yao

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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