> From: Stephen Hemminger [mailto:[email protected]]
> Sent: Tuesday, 20 January 2026 21.00
>
> On Tue, 20 Jan 2026 10:17:01 +0000
> Morten Brørup <[email protected]> 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 <[email protected]>
>
> 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.
