Remove string allocations

[kerams]

No description provided.

[cartermp]

The other two are obvious but I was curious about the removal of substring and use of span. This benchark: https://gist.github.com/cartermp/da23761991bd1cf0f179edfe1d00b737

yielded these results:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.388 (2004/?/20H1)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-preview.6.20318.15
  [Host]        : .NET Core 3.1.6 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.31603), X64 RyuJIT DEBUG
  .NET 4.7.2    : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
  .NET Core 3.1 : .NET Core 3.1.6 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.31603), X64 RyuJIT

Method	Job	Runtime	name	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
IsValidStrippedName	.NET 4.7.2	.NET 4.7.2	abc123thxypo	37.52 ns	0.484 ns	0.429 ns	1.00	0.0134	-	-	56 B
IsValidStrippedNameSpan	.NET 4.7.2	.NET 4.7.2	abc123thxypo	26.21 ns	0.153 ns	0.143 ns	0.70	-	-	-	-
IsValidStrippedName	.NET Core 3.1	.NET Core 3.1	abc123thxypo	29.98 ns	0.321 ns	0.300 ns	1.00	0.0114	-	-	48 B
IsValidStrippedNameSpan	.NET Core 3.1	.NET Core 3.1	abc123thxypo	19.67 ns	0.149 ns	0.125 ns	0.66	-	-	-	-
IsValidStrippedName	.NET 4.7.2	.NET 4.7.2	xyz	35.00 ns	0.328 ns	0.307 ns	1.00	0.0076	-	-	32 B
IsValidStrippedNameSpan	.NET 4.7.2	.NET 4.7.2	xyz	26.49 ns	0.181 ns	0.170 ns	0.76	-	-	-	-
IsValidStrippedName	.NET Core 3.1	.NET Core 3.1	xyz	28.86 ns	0.489 ns	0.458 ns	1.00	0.0076	-	-	32 B
IsValidStrippedNameSpan	.NET Core 3.1	.NET Core 3.1	xyz	18.77 ns	0.172 ns	0.161 ns	0.65	-	-	-	-

(note: github struggles to render the actual proper values in tables)

[cartermp]

Great changes, thanks!

[TIHan]

The changes look good, but I would like to see more information regarding these changes.

Do the changes actually give a measurable performance improvement in the overall tooling? Can you provide evidence?

Also, more description of the PR would be helpful too instead of it being empty.

Normally I would 'request changes', but since this isn't touching the internals of the compiler - I can be a bit more relaxed.

[TIHan]

To note: The only reason why I made the comment that I did was because we have to use AsSpan and depending on the case, can make me a little nervous in terms of readability if we start doing that everywhere.

[kerams]

I was just browsing the repo and noticed these things could be improved, so that's what I did thinking any performance uplift (and I'm pretty sure this change will have a completely negligible impact on its own) is better than nothing, admittedly with no regard to readability. That's all there is to say, really ;)

[KevinRansom]

These are good changes. I agree the perf improvements will be minuscule, we normally try to avoid drive-by churn because of unintended consequences, however, these look focused enough that the risk is pretty low.

Thank you for taking care of this

Kevin

[cartermp]

These changes may not actually be miniscule - one of these cases removes a new string allocation for every single span of text we classify in a document. For large documents that could be meaningful.

[kerams]

@cartermp, how would one go about measuring the difference on a sizeable file/codebase?

[cartermp]

Measuring the difference is best done with PerfView and capturing a trace before/after your changes for the same codebase and operations done in the codebase. It's kind of annoying and time-consuming work. In general I think this is fine to take without advanced analysis since the improvements are pretty obvious. But for less obvious things we would at least need to see some more information about the current performance characteristics and a writeup about how the new code improves on it.