On Tue, Sep 03, 2024 at 10:25:10PM -0400, Liam R. Howlett wrote: >* 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? > 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
