Re: [PATCH 1/3] maple_tree: use ma_data_end() in mas_data_end()

[Wei Yang <richard.weiyang@gmail.com>]

On Tue, Sep 03, 2024 at 10:25:10PM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [240903 20:15]:
>> On Tue, Sep 03, 2024 at 12:12:39PM -0400, Liam R. Howlett wrote:
>> >* Wei Yang <richard.weiyang@gmail.com> [240830 20:11]:
>> >> These two function share almost the same code and do the same thing.
>> >
>> >Please stop trying to optimise code like this.  I don't have time to
>> >verify you are not making things worse and you haven't done any of the
>> >testing required.
>> >
>> >I went through with perf to optimise dereferences and static inline
>> >functions, so unless you are prepared to look at the generated code and
>> >actually benchmark, please stop sending patches that I need to verify
>> >like this myself.
>> >
>> 
>> Would you mind letting me know how could I verify the generated code and
>> benchmark? What you expect to do?
>

Liam,

Thanks for your patient reply.

>There are a number of benchmarks in the test code that will run specific
>tasks in iterations to check that there is not a regression.  Depending
>on what you change, you have to run the right one.  Usually I write one
>if one doesn't exist.

Not familiar with benchmarks. Would you mind naming someone? or the ones you
have written? So that I can learn to test or write one later.

>
>You can also use perf record and see what happens on specific tests.
>
>This function is all over the place, so I really don't know which one to
>run, and I don't have time to write a test for your changes - and I
>don't think this is worth it.
>
>> 
>> For example this one, these two functions share the same code.
>
>Do they?
>
>You have removed the fast path for maple_arange_64 using the metadata
>end before getting the pivots, which means on basically all vma walks we
>will be adding instructions to the walking of the tree (get the pivots,
>check if the node is dead, check if the pivots pointer is null).  You
>have also added a check for if (!pivots), which is essentially checking
>if a dead node was hit - which is already checked in the mas_data_end().
>These two checks are the reasons I left both copies of this function in
>place. I am looking to reduce the execution time, not decrease the line
>count of the file.
>

You are right, I should pay more attention to the fast path opt.

>So, the last 8 lines of both functions are the same. And please don't
>submit a patch that adds an inline function that does the last 8 lines
>to reduce duplication.
>

Ok, I will not try to call an inline function just to reduce some similar
code.

>> What's your
>> concern for this? The inline function expansion?
>
>Your clean up is compressing setting variables to the same line, which
>is a bad change.  It is better to have verbose code where it won't make
>a difference in what is compiled.  And certainly not worth adding after
>the fact.
>
>The inline of the mas_data_end() function depends on the expansion
>happening by the compiler (which might change based on the version or if
>it's gcc vs clang), sure that's a bit of a concern.
>
>The biggest annoying thing about this whole patch series (without a
>cover letter) is that it isn't doing anything for fixing, helping, or
>adding functionality.  I'm a big fan of forward progress and this isn't
>it.
>
>It is only changing code for the sake of changing code.  And it looks
>like it will be slower, or the same speed if we are lucky.  I have to
>take time to verify things aren't slower or add subtle issues (maybe an
>RCU race) because the code looked similar.  It's just not worth it.
>

I am trying to make the code more easy to read, but seems not helping.

BTW, I found in mas_update_gap(), if (p_gap != max_gap), we would access
the first parent, parent's type and its gap twice. Once in mas_update_gap()
and once in mas_parent_gap().

Do you think it worth a change to reduce one?

-- 
Wei Yang
Help you, Help me