Re: [PATCH 3/3] perf report: Add weight[123] output fields

[Namhyung Kim]

On Tue, Apr 9, 2024 at 11:18 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 2024-04-09 12:53 p.m., Namhyung Kim wrote:
> > Hi Kan,
> >
> > On Tue, Apr 9, 2024 at 9:37 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2024-04-08 8:06 p.m., Namhyung Kim wrote:
> >>> Add weight1, weight2 and weight3 fields to -F/--fields and their aliases
> >>> like 'ins_lat', 'p_stage_cyc' and 'retire_lat'.  Note that they are in
> >>> the sort keys too but the difference is that output fields will sum up
> >>> the weight values and display the average.
> >>>
> >>> In the sort key, users can see the distribution of weight value and I
> >>> think it's confusing we have local vs. global weight for the same weight.
> >>>
> >>> For example, I experiment with mem-loads events to get the weights.  On
> >>> my laptop, it seems only weight1 field is supported.
> >>>
> >>>   $ perf mem record -- perf test -w noploop
> >>>
> >>> Let's look at the noploop function only.  It has 7 samples.
> >>>
> >>>   $ perf script -F event,ip,sym,weight | grep noploop
> >>>   # event                         weight     ip           sym
> >>>   cpu/mem-loads,ldlat=30/P:           43     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=30/P:           48     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=30/P:           38     55b3c122bffc noploop    <--- same weight
> >>>   cpu/mem-loads,ldlat=30/P:           38     55b3c122bffc noploop    <--- same weight
> >>>   cpu/mem-loads,ldlat=30/P:           59     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=30/P:           33     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=30/P:           38     55b3c122bffc noploop    <--- same weight
> >>>
> >>> When you use the 'weight' sort key, it'd show entries with a separate
> >>> weight value separately.  Also note that the first entry has 3 samples
> >>> with weight value 38, so they are displayed together and the weight
> >>> value is the sum of 3 samples (114 = 38 * 3).
> >>>
> >>>   $ perf report -n -s +weight | grep -e Weight -e noploop
> >>>   # Overhead  Samples  Command   Shared Object   Symbol         Weight
> >>>        0.53%        3     perf   perf            [.] noploop    114
> >>>        0.18%        1     perf   perf            [.] noploop    59
> >>>        0.18%        1     perf   perf            [.] noploop    48
> >>>        0.18%        1     perf   perf            [.] noploop    43
> >>>        0.18%        1     perf   perf            [.] noploop    33
> >>>
> >>> If you use 'local_weight' sort key, you can see the actualy weight.
> >>>
> >>>   $ perf report -n -s +local_weight | grep -e Weight -e noploop
> >>>   # Overhead  Samples  Command   Shared Object   Symbol         Local Weight
> >>>        0.53%        3     perf   perf            [.] noploop    38
> >>>        0.18%        1     perf   perf            [.] noploop    59
> >>>        0.18%        1     perf   perf            [.] noploop    48
> >>>        0.18%        1     perf   perf            [.] noploop    43
> >>>        0.18%        1     perf   perf            [.] noploop    33
> >>>
> >>> But when you use the -F/--field option instead, you can see the average
> >>> weight for the while noploop funciton (as it won't group samples by
> >>
> >> %s/funciton/function/
> >>
> >>> weight value and use the default 'comm,dso,sym' sort keys).
> >>>
> >>>   $ perf report -n -F +weight | grep -e Weight -e noploop
> >>>   # Overhead  Samples  Weight1  Command  Shared Object  Symbol
> >>>        1.23%        7     42.4  perf     perf           [.] noploop
> >>
> >> I think the current +weight shows the sum of weight1 of all samples,
> >> (global weight). With this patch, it becomes an average (local_weight).
> >> The definition change may break the existing user script.
> >>
> >> Ideally, I think we should keep the meaning of the weight and
> >> local_weight as is.
> >
> > Hmm.. then we may add 'avg_weight' or something.
> >
> > But note that there's a subtle difference in the usage.  If you use
> > 'weight' as a sort key (-s weight) it'd keep the existing behavior
> > that shows the sum (global_weight).  It'd show average only if
> > you use it as an output field (-F weight).
> >
>
> As my understanding, the -F weight is implicitly replaced by the -F
> weight1 with this patch. There is no way to get the sum of weight with
> -F anymore.

Right.

>
> I think that's a user visible behavior change. At least, we have to warn
> the end user with a message, e.g., "weight is not supported with -F
> anymore. Using weight1 to instead". Only updating the doc may not be enough.

I understand your concern.  I can add the warning.

>
> > The issue of the sort key is that it cannot have the total sum
> > of weights for a function.  It'll have separate entries for each
> > weight for each function like in the above example.
> >
>
> That seems to be a different issue. If the total sum of weights for a
> function is required, we should fix the existing "weight".

Yeah, I guess that's more reasonable behavior.  But I'm not sure
how we can fix it without breaking the existing behavior.

Actually this is my approach to keep the behavior for the "sort" key.
I think users are more familiar with -s (--sort) rather than the -F
(--fields) option.  That's why I'd like to "break" that part only.

Thanks,
Namhyung