CoreFx #22409 StreamWriter Span based API and tests #23915
Conversation
| @@ -555,6 +571,11 @@ public override void WriteLine(string value) | |||
| } | |||
| } | |||
|
|
|||
| public override void WriteLine(ReadOnlySpan<char> source) | |||
| { | |||
| WriteLine(new string(source.ToArray())); | |||
There was a problem hiding this comment.
This is very inefficient, allocating both a char[] and a string. At the very least a better implementation would be:
Write(source);
WriteLine();but even better would be like the one that takes a string, except using the ReadOnlySpan<char> as the source, and unless there's a measurable perf regression we can't overcome, both this and the string-based overload should just delegate to the same private ReadOnlySpan<char>-based helper that has that logic. This one would then also do a check to make sure it's a concrete StreamWriter, and if it's not, just delegate to the base, e.g.
if (GetType() == typeof(StreamWriter))
{
WriteLineCore(source);
}
else
{
base.WriteLine(source);
}There was a problem hiding this comment.
Thanks @stephentoub . Actually I had changed WriteLine(string) to use ReadOnlySpan and then make existing string based overload delegate to it... But after we discussed about TextReader/Writer to use rented array, I thought that is something to be done across the board!
There was a problem hiding this comment.
I thought that is something to be done across the board!
TextReader/Writer are abstract base classes: these methods needed to delegate there to the existing virtuals in order for them to get the correct behavior from existing overrides. But StreamReader/Writer are derived classes with their own implementations; as long as the instance is a concrete StreamReader/Writer and not something derived from it, we want them to have the most efficient implementations possible (and for the case where it's a derived type and thus where we still need to call an existing virtual method that may have been overridden), we just delegate to the base methods on TextReader/Writer that already do the appropriate delegation.
| { | ||
| ArrayPool<char>.Shared.Return(buffer); | ||
| } | ||
| } |
There was a problem hiding this comment.
Similarly, this is inefficient. The implementation should look like that in Write(string), and ideally if there's no measurable perf regression from doing so, they should share an implementation in a helper, with this doing a type check and delegating to the base if it's not a concrete StreamWriter. The base already does this ArrayPool interaction.
There was a problem hiding this comment.
Yup got it! thanks! I'm reverting the whole stuff, reinstate the previous WriteLine overload and do perf measurement for the String overload using the two ways - original and delegation to ReadOnlySpan based overload...
|
@stephentoub I have pushed a newer implementation as per review comments. It is resulting into performance degradation for existing tests that work with entire char buffer of short length and, partial buffer of long or short length, though the Please note that this doesn't have perf test for Looks like I will have to work on improving the performance. Appreciate your inputs. |
|
@stephentoub I am not able to understand how to settle down for a particular performance reading. I removed all the changes and reran the perf test for Original (2 days back)
ReRun (without any changes)
|
It could be that the tests aren't particularly stable, meaning there's too much variation and work needs to be done to reign in the results. It could also be that you had other things happening on your machine the two times you ran them, perturbing the results. |
WinCPP
force-pushed
the
span_issue_22409
branch
from
61c8be0
to
d3ac17e
Compare
|
Resuming work... rebased from the dotnet/master... Please ignore latest commit... it is just to sync the branch. |
Thanks :) |
|
Hi @stephentoub I rebased and now when I try to run the perf test, it gives me an error as below. I have posted the same on gitter as well, just in case someone knows or has encountered it. But I did nothing special, just rebased to get in merged code for #22406 and other stuff. I have even tried .\clean.cmd -b -c -p to get all the packages once again. Still no luck... Appreciate your advise! Thanks! |
|
@WinCPP, this is most likely https://github.com/dotnet/corefx/issues/24153. |
WinCPP
force-pushed
the
span_issue_22409
branch
from
d3ac17e
to
0d8a4c2
Compare
|
@stephentoub following is the latest numbers for performance. Looks like performance for all the
With Core function
Please note that there are no performance tests for |
|
@dotnet-bot test Linux arm Release Build please |
WinCPP
force-pushed
the
span_issue_22409
branch
from
9713dca
to
287c30a
Compare
|
@stephentoub , as mentioned in previous comment here, following is the data for the alternate implementation in which the
Observation - performance for existing functions almost remains unaltered. Although, the Looks like making a copy of core code (as per practice in the file), instead of calling a core code method, appears to be the better approach for adding two new overloads? Appreciate your thoughts. Notes:
Original performance data (without implementing the
After adding
|
|
Thanks. I would like us to try to avoid duplicating the code. I will try to look tomorrow. |
|
Thank you! To check the implementation that uses duplicated code, please refer here. This is a temp branch in which I have parked the code. |
|
Just FYI, I haven't forgotten about this... just had to switch to some other things, and I'll get back to it soon... |
|
Yup I understand! Thank you! In the meantime I am working |
|
Hi @stephentoub I was wondering if this will require exposing some new overload such as source.Slice(index, n).CopyTo(new Span<char>(_charBuffer, _charPos, n));I suspect creating slices of I believe this is more involved stuff than it appears and might require some deeper knowledge, so would you mind taking taking over this PR and associated issue #22409...? I will move to something simpler. Thanks! |
No problem. Thanks for your efforts. |
This PR is for the Span based API for StreamWriter. Please note that this PR is dependent on PR #23786 for successful compilation.
@stephentoub @danmosemsft kindly review.
Fixes #22409.
Thanks,
Mandar