Add support for IUtf8SpanFormattable & other updates #27
Conversation
- Use HtmlEncoder in request services if explicit one isn't provided
DamianEdwards
force-pushed
the
IUtf8SpanFormattable
branch
from
fc4b70e to
eda1165
Compare
| else if (value is IUtf8SpanFormattable) | ||
| { | ||
| WriteUtf8SpanFormattable((IUtf8SpanFormattable)(object)value, default, null); |
There was a problem hiding this comment.
nit:
else if (value is IUtf8SpanFormattable formattable)
{
WriteUtf8SpanFormattable(formattable, default, null);
Saves an unnecessary castclass call in IL, although JIT might be smart enough to remove it?
There was a problem hiding this comment.
I was told this is the pattern to use as JIT supports it and will avoid the boxing, etc.
Cc @stephentoub
There was a problem hiding this comment.
I was told this is the pattern to use as JIT supports it and will avoid the boxing, etc.
That's correct. See the difference between M1 and M2 here:
SharpLab
There was a problem hiding this comment.
Huh, that's interesting. Any reason the JIT can't produce the same code in both cases?
| { | ||
| var buffer = ArrayPool<char>.Shared.Rent(value.Length); | ||
| value.CopyTo(buffer); | ||
| htmlEncoder.Encode(textWriter, buffer, 0, value.Length); |
There was a problem hiding this comment.
Looks like an opportunity for a new API? Renting and copying is only needed because there is an API lacking for ROS<char>
There was a problem hiding this comment.
API like TextEncoder.Encode(TextWriter, ReadOnlySpan<char>)?
There was a problem hiding this comment.
@stephentoub thoughts?
There was a problem hiding this comment.
We could possibly add a TextWriter, ReadOnlySpan<char> overload, but note that the string/char[] overloads themselves just turn around and rent an array from the pool (they also need temporary space into which to encode before then writing that encoded output to the writer).
HtmlEncoder does have an Encode that takes a ReadOnlySpan<char>, it just writes to a destination span rather than to a TextWriter.
@GrabYourPitchforks, did we consider adding a convenience span-based Encode with TextWriter? I see one mentioned in dotnet/runtime#27919 but that was closed.
| { | ||
| renderTask.GetAwaiter().GetResult(); | ||
| return httpContext.Response.BodyWriter.FlushAsync().AsTask(); | ||
| return httpContext.Response.BodyWriter.FlushAsync(httpContext.RequestAborted).AsTask(); |
There was a problem hiding this comment.
I think this can allocate since it'll create a Task<FlushResult> even though we're throwing away the result. See https://github.com/dotnet/aspnetcore/blob/main/src/Shared/ValueTaskExtensions/ValueTaskExtensions.cs for what we do in ASP.NET Core when ignoring the result to avoid the allocation.
IUtf8SpanFormattablein net8.0HtmlEncoderfrom DI if one isn't explicitly provided toRazorSliceHttpResultTextWriteroutput supportCancellationTokentoRenderAsyncmethodsFixes #15