Re: [PATCH v2] mempool: simplify get objects

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 20 Jan 2026 10:17:01 +0000
Morten Brørup <mb@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@smartsharesystems.com>

Not sure I agree with what AI review shows, but here it is:


## 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

## 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.

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.

## 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.