[PATCH v2] mempool: simplify get objects

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Tuesday, 20 January 2026 21.00
> 
> On Tue, 20 Jan 2026 10:17:01 +0000
> Morten Brørup <mb at smartsharesystems.com> wrote:
> 
> > Removed explicit test for build time constant request size,
> > and added comment that the compiler loop unrolls when request size is
> > build time constant, to improve source code readability.
> >
> > Signed-off-by: Morten Brørup <mb at smartsharesystems.com>
> 
> Not sure I agree with what AI review shows, but here it is:

In this case, I consider the AI review very interesting; it tells a lot about the AI.
Thank you, Stephen!

> 
> 
> ## Commit Message Review
> 
> **✓ Good:**
> - Subject line: 29 characters (well under 60 limit)
> - Proper component prefix: `mempool:`
> - Lowercase, imperative mood, no trailing period
> - Body properly wrapped under 75 characters
> - Has `Signed-off-by:` with real name and email
> - Version changelog correctly placed after `---`
> 
> **⚠ Warnings:**
> - No mention of testing or performance validation
> - Missing justification for why this simplification is beneficial

My description has "to improve source code readability", which I consider the benefit.

> 
> ## Code Style Review
> 
> **✓ Good:**
> - Comment formatting follows DPDK conventions
> - Lines under 100 characters
> - No trailing whitespace visible
> - Consistent indentation
> 
> **⚠ Code Logic Concerns:**
> 
> 1. **Removed optimization paths**: The patch removes
> `__rte_constant(n)` checks that allowed the compiler to optimize for
> build-time constant request sizes. While the comment claims "the
> compiler unrolls the fixed length copy loop," removing the explicit
> constant checks may reduce optimization effectiveness.
> 
> 2. **Removed early return**: The old code had an optimization for the
> case where `remaining == 0` (entire request satisfied from partial
> cache use):
>    ```c
>    if (!__rte_constant(n) && likely(remaining == 0)) {
>        /* stats and return */
>    }
>    ```
>    This early return is now removed, which could slightly impact
> performance for this common case.

For 1 and 2, it seems the AI is not clever enough to realize that the resulting code is exactly the same with or without these changes.
So the AI worries if there might be a performance impact, and mentions it here.

And since the AI thinks there is a potential performance impact, it correctly added the warning bullet about "No mention of testing or performance validation".
I guess that is also why the AI asks for justification (as a separate warning bullet).

> 
> 3. **Unnecessary code movement**: Line 127 moves `cache->len = cache-
> >size;` before the loop when it was previously after. Since the loop
> doesn't use `cache->len`, this change appears cosmetic but reduces code
> clarity - it's less obvious why the assignment happens before consuming
> the objects.

It is cosmetic; for code similarity, cache->len is now set before each copy loop.
I would consider making three instances of some code look similar an improvement.

On the other hand, we generally don't like unnecessary code changes. And this could look like one.

But it's not only cosmetic.
It's also an micro-optimization, which I (to reduce noise) omitted to mention in the description:
The function's pointer parameters are not marked "restrict", so writing to "obj_table" in the copy loop might formally modify cache->size. And thus, setting cache->len = cache->size after the copy loop requires loading cache->size again after copying the objects.
Moving this line up before the copy loop avoids that extra load of cache->size when setting cache->len.

The AI is not clever enough to realize this.
Which probably also means that it is not clever enough to suggest such optimizations.

> 
> ## Missing Elements
> 
> **⚠ Should address:**
> - No performance testing mentioned for a patch that removes
> optimizations
> - No explanation of why simplification is worth potential performance
> impact
> - Consider benchmarking showing no regression
> 
> ## Recommendations
> 
> 1. **Enhance commit message** with performance testing results to
> justify the simplification
> 2. **Consider keeping** the early return at line 111-118 for the
> `remaining == 0` case as it's a worthwhile optimization
> 3. **Reconsider** moving `cache->len = cache->size;` - leave it in the
> original position for clarity
> 4. **Add rationale** explaining why removing `__rte_constant(n)` checks
> doesn't harm performance
> 
> The patch compiles and maintains functional correctness, but the
> performance implications of removing compiler optimization hints should
> be validated and documented.