[chore] Fix MergeSplit logic, update tests to use Request

[bogdandrutu]

No need for a changelog since the cache items was added this release.

Depends on #12169

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.82%. Comparing base (ad36545) to head (7e52446).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12170      +/-   ##
==========================================
- Coverage   91.82%   91.82%   -0.01%     
==========================================
  Files         463      463              
  Lines       24776    24773       -3     
==========================================
- Hits        22750    22747       -3     
  Misses       1644     1644              
  Partials      382      382              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jmacd]

Can we eliminate cached item counts? I wonder if it's a premature optimization.

[jmacd]

This makes me wonder what the real benefit of having a cached items count is? How many times do we expect that value to be read, aside from once by the observability sender?

[sfc-gh-sili]

Hi Joshua! This optimization comes from the observation that ItemsCount() is repeatedly called during merge-splitting and is a fairly expensive operation. The optimization has proved to save 30 - 75% cpu time for merging based on the benchmark (see more details in #12136)

I understand your concern around having an additional variable to book-keep could be error-prone, but I would argue that no one, except for the batcher, should be messing around with the request once its created. We just need to make sure that the cached items count is updated when the wrapped proto is updated.

[bogdandrutu]

Can we eliminate cached item counts? I wonder if it's a premature optimization.

I would prefer not to #12136