2020-10-12 17:10:37

by Alexey Budankov

[permalink] [raw]
Subject: Re: [PATCH v1 02/15] perf report: output trace file name in raw trace dump


On 12.10.2020 19:01, Andi Kleen wrote:
> On Mon, Oct 12, 2020 at 11:54:24AM +0300, Alexey Budankov wrote:
>> @@ -180,7 +183,8 @@ static int ordered_events__deliver_event(struct ordered_events *oe,
>> ordered_events);
>>
>> return perf_session__deliver_event(session, event->event,
>> - session->tool, event->file_offset);
>> + session->tool, event->file_offset,
>> + event->file_path);
>
> Wouldn't it be better to pass "event" around now, which would contain at least
> four of the arguments.
>
> These functions are getting entirely too many arguments.

Well, either approach is possible, and even shrink of two arguments kept at session object.
However changing function signature more than posted can cause bigger adjustments all over
the code. So this needs more evaluation prior implementation.

Alexei


2020-10-21 06:36:03

by Alexey Budankov

[permalink] [raw]
Subject: Re: [PATCH v1 02/15] perf report: output trace file name in raw trace dump


On 12.10.2020 20:06, Alexey Budankov wrote:
>
> On 12.10.2020 19:01, Andi Kleen wrote:
>> On Mon, Oct 12, 2020 at 11:54:24AM +0300, Alexey Budankov wrote:
>>> @@ -180,7 +183,8 @@ static int ordered_events__deliver_event(struct ordered_events *oe,
>>> ordered_events);
>>>
>>> return perf_session__deliver_event(session, event->event,
>>> - session->tool, event->file_offset);
>>> + session->tool, event->file_offset,
>>> + event->file_path);
>>
>> Wouldn't it be better to pass "event" around now, which would contain at least
>> four of the arguments.
>>
>> These functions are getting entirely too many arguments.
>
> Well, either approach is possible, and even shrink of two arguments kept at session object.
> However changing function signature more than posted can cause bigger adjustments all over
> the code. So this needs more evaluation prior implementation.

After brief evaluation it still doesn't look easy. The simplest thing
I could imagine is to probably combine file_path and file_offset at a
struct object on stack and then pass the object by the reference along
function calls. I expect it will roughly cause the same amount of changes
in the code and saves one argument (GP register). It is not that much
but still. However I don't see issues with passing even 6 args on stack.

Alexei

2020-10-21 06:46:49

by Alexey Budankov

[permalink] [raw]
Subject: Re: [PATCH v1 02/15] perf report: output trace file name in raw trace dump


On 20.10.2020 19:31, Alexey Budankov wrote:
>
> On 12.10.2020 20:06, Alexey Budankov wrote:
>>
>> On 12.10.2020 19:01, Andi Kleen wrote:
>>> On Mon, Oct 12, 2020 at 11:54:24AM +0300, Alexey Budankov wrote:
>>>> @@ -180,7 +183,8 @@ static int ordered_events__deliver_event(struct ordered_events *oe,
>>>> ordered_events);
>>>>
>>>> return perf_session__deliver_event(session, event->event,
>>>> - session->tool, event->file_offset);
>>>> + session->tool, event->file_offset,
>>>> + event->file_path);
>>>
>>> Wouldn't it be better to pass "event" around now, which would contain at least
>>> four of the arguments.
>>>
>>> These functions are getting entirely too many arguments.
>>
>> Well, either approach is possible, and even shrink of two arguments kept at session object.
>> However changing function signature more than posted can cause bigger adjustments all over
>> the code. So this needs more evaluation prior implementation.
>
> After brief evaluation it still doesn't look easy. The simplest thing
> I could imagine is to probably combine file_path and file_offset at a
> struct object on stack and then pass the object by the reference along
> function calls. I expect it will roughly cause the same amount of changes
> in the code and saves one argument (GP register). It is not that much
> but still. However I don't see issues with passing even 6 args on stack.

Sorry - "passing 6 args to a function call"

2020-10-21 10:04:01

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH v1 02/15] perf report: output trace file name in raw trace dump

> > After brief evaluation it still doesn't look easy. The simplest thing
> > I could imagine is to probably combine file_path and file_offset at a
> > struct object on stack and then pass the object by the reference along
> > function calls. I expect it will roughly cause the same amount of changes
> > in the code and saves one argument (GP register). It is not that much
> > but still. However I don't see issues with passing even 6 args on stack.
>
> Sorry - "passing 6 args to a function call"

Ah it wasn't about code efficiency, just maintainability. Function calls
with too many arguments are generally hard to maintain, and typically
at some point have to be refactored anyways because they tend to grow
even more over time.

But if it's too difficult, ok.

-Andi