* Wei Yang <richard.weiyang@xxxxxxxxx> [240903 20:15]:
> On Tue, Sep 03, 2024 at 12:12:39PM -0400, Liam R. Howlett wrote:
> >* Wei Yang <richard.weiyang@xxxxxxxxx> [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?
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.
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.
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.
> 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.
>
> >>
> >> Let's just call ma_data_end() in mas_data_end() to reduce duplicate
> >> code.
> >>
> >> Signed-off-by: Wei Yang <richard.weiyang@xxxxxxxxx>
> >> ---
> >> lib/maple_tree.c | 22 ++++------------------
> >> 1 file changed, 4 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> >> index b7d747a7938e..85668246f944 100644
> >> --- a/lib/maple_tree.c
> >> +++ b/lib/maple_tree.c
> >> @@ -1435,28 +1435,14 @@ static __always_inline unsigned char ma_data_end(struct maple_node *node,
> >> */
> >> static inline unsigned char mas_data_end(struct ma_state *mas)
> >> {
> >> - enum maple_type type;
> >> - struct maple_node *node;
> >> - unsigned char offset;
> >> - unsigned long *pivots;
> >> -
> >> - type = mte_node_type(mas->node);
> >> - node = mas_mn(mas);
> >> - if (type == maple_arange_64)
> >> - return ma_meta_end(node, type);
> >> + enum maple_type type = mte_node_type(mas->node);
> >> + struct maple_node *node = mas_mn(mas);
> >> + unsigned long *pivots = ma_pivots(node, type);
> >>
> >> - pivots = ma_pivots(node, type);
> >> if (unlikely(ma_dead_node(node)))
> >> return 0;
> >>
> >> - offset = mt_pivots[type] - 1;
> >> - if (likely(!pivots[offset]))
> >> - return ma_meta_end(node, type);
> >> -
> >> - if (likely(pivots[offset] == mas->max))
> >> - return offset;
> >> -
> >> - return mt_pivots[type];
> >> + return ma_data_end(node, type, pivots, mas->max);
> >> }
> >>
> >> /*
> >> --
> >> 2.34.1
> >>
> >>
>
> --
> Wei Yang
> Help you, Help me
