SOLR-16845 BinaryResponseWriter should not attempt cast to Utf8CharSequence

[stillalex]

https://issues.apache.org/jira/browse/SOLR-16845

Description

Removing the unneeded call to StoredField#getCharSequenceValue for every stored field in the query response.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added documentation for the Reference Guide

[stillalex]

@HoustonPutman fyi

[epugh]

For changes like this, I wish there was a way for me, as someone who hasn't really worked deeply in this code base to know that this is a positivie safe change.. When folks add features and add new unit tests, those tests demonstrate that the change is safe and good... In this case, where you are refactoring/simplifying, how can we have general confidence in the change? <--- This is kind of a more general question.... For example, is there a set of tests you could run and attach some sort of data that shows that this change doesnt' have any side effects or performance issues?

I know this is also a topic to bring up at our next Community meeting.... How to do performance testing ;-)

[stillalex]

For changes like this, I wish there was a way for me, as someone who hasn't really worked deeply in this code base to know that this is a positivie safe change

I agree with the sentiment. I would also like to have some confirmation that this is a safe change :)
Trouble here is, as far as I can read it, everything inside the if statement is dead code, so having some sort of code coverage report would probably show this, but I am not aware of anything doing this in Solr. This comes from a partial rollback, I provided a few links in the Jira itself. The test itself is not exercising a real case scenario, it is making up some SolrDocument in a way that does not exist in the 'productive' code anymore, so I removed it too.

[epugh]

Does the "beasting" technique help on building confidence on this?

I really really want to click the "Squash and Merge" button.... ;-)

[stillalex]

fixed a tidy problem and pushed again

[stillalex]

I recently added some benchmarks in this area (SOLR-16841), and I would really hope people look at them and validate the data patters (number of docs, size, threads and so on). but as they stand, here are the numbers on main branch vs this change:

./jmh.sh QueryResponseWriters -p wt=javabin

• main branch

Iteration   1: 3135.579 ops/s
Iteration   2: 3137.206 ops/s
Iteration   3: 3123.397 ops/s
Iteration   4: 3137.906 ops/s

Benchmark                      (wt)   Mode  Cnt     Score    Error  Units
QueryResponseWriters.query  javabin  thrpt    4  3133.522 ± 44.073  ops/s

• this PR

Iteration   1: 3312.928 ops/s
Iteration   2: 3303.225 ops/s
Iteration   3: 3321.268 ops/s
Iteration   4: 3320.655 ops/s

Benchmark                      (wt)   Mode  Cnt     Score    Error  Units
QueryResponseWriters.query  javabin  thrpt    4  3314.519 ± 54.486  ops/s

so I would say ~5% improvement

[stillalex]

@epugh posted some perf numbers, but I think the confidence we are looking for is more in the correctness side, rather than speed :) so probably someone more familiar with the area could validate these changes.

[epugh]

Let's give it a day or two to see if anyone weighs in, and then happy to commit it! The perf numbers are nice ;-)

[stillalex]

@epugh no updates on this one from anyone else. should I shoot an email to dev list to check is anyone has any strong opinions against merging?

[epugh]

Please do! I am out for the next week, so if you don't hear back in the upcoming week, then do ping me.

[HoustonPutman]

This looks good to me. It is confusing which parts of that reverted patch are still good and which aren't. But I'm glad you came with performance tests.

I see how both of these pieces of code aren't used anymore.

Please add a changelog entry for 9.4, you can put (via Houston Putman) since I'm going to be committing it.

[stillalex]

thank you for taking a look @HoustonPutman!
sorry I did a few force push trying to update the changes file. should be good now.