Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Don't we normally wait for API approval before implementing? Or was this scratching a particular itch 😄

Don't we normally wait for API approval before implementing?

Before putting up a PR, yes :-(

The API was quasi-approved during the Jan 14 review. The only work item from that review was to make sure that callers didn't need TrimStart or TrimEnd in addition to simply Trim. I investigated and found that no callers would benefit from those extra enum entries.

So now that all concerns have been addressed the only thing that's left for API review is approval. Modulo naming if somebody wants a last-minute enum name change. :)

I see dotnet-runtime-perf ran -- @DrewScoggins would this give us results for perf tests, or just verify the infrastructure didn't break?

Probably should check perf impact on String.Split.

I still need to investigate why some unit tests are failing. The report says that 19 failures occurred, but every test is marked "Pass". So that's fun. :D

I didn't perf test this but I don't anticipate much perf impact. The related PR #35733 should slightly improve perf in the RemoveEmptyEntries case since it more efficiently resizes the final returned array.

Separately, I was experimenting with a design that removes the intermediate array entirely. That should result in less GC pressure in the RemoveEmptyEntries case. But since it introduces some level of complexity and isn't a visible behavioral change I thought it best to reserve that improvement for a different PR.

Those are just correctness perf tests, so not taking real measurements. If we expect some kind of perf impact, either a regression or an improvement, I would recommend that you run performance tests locally. We have this guide, https://github.com/dotnet/performance/blob/master/docs/benchmarking-workflow-dotnet-runtime.md, that should help you get started. If you get stuck on anything feel free to contact either Adam or I. Also once these changes go in we will get lab results and we can take a look at the impact that we are measuring there.

Thanks @DrewScoggins I should have realized that (in fact I'm sure you told me that already 😸 )

I'm going to back out the "Remove Utf8StringSplitOptions and redirect all of the Utf8String infrastructure to the improved StringSplitOptions type" part of the PR for now. I didn't fully think through the implications this would have on the downlevel package.

Unit tests were failing due to an off-by-one error in a boundary condition. It's addressed at in the commit 5b07dbc, which is part of this PR. Aside from this and reverting the Utf8StringSplitOptions change, the logic is unchanged from the earlier code review.

crossgen2 CI failure is #37732