Port System.Net.WebUtility missing members by safern · Pull Request #13875 · dotnet/corefx

safern · 2016-11-22T00:58:14Z

Fixes #12142

cc @danmosemsft @AlexGhiondea @stephentoub @geoffkizer

danmoseley · 2016-11-22T01:19:50Z

src/System.Runtime.Extensions/src/System/Net/WebUtility.cs


+        public static void HtmlEncode(string value, TextWriter output)
+        {
+            // Since we already have a private method that decodes the value using a StringBuilder we don't want to have duplicate code in order to have a better mantainance.


Nit, I wouldn't bother with the comments, since they only make sense if you know there was previously an explicit implementation in terms of TextWriter. Up to you.

justinvp · 2016-11-22T03:33:02Z

src/System.Runtime.Extensions/src/System/Net/WebUtility.cs

+        public static void HtmlEncode(string value, TextWriter output)
+        {
+            // Since we already have a private method that decodes the value using a StringBuilder we don't want to have duplicate code in order to have a better mantainance.
+            output.Write(HtmlEncode(value));


This is sufficient to bring the functionality to .NET Core, but it can be improved... In desktop, the encoded chars are written directly to the TextWriter, whereas with this, a (potentially large) string is being allocated and then that string is written to the TextWriter. It'd be nice to avoid the string allocation.

In desktop, the shared implementation is in HtmlEncode(string, TextWriter) and HtmlEncode(string) allocates a StringWriter to pass to HtmlEncode(string, TextWriter). However, in .NET Core, since the other overload wasn't present, HtmlEncode(string) was changed to use StringBuilder (acquired from a StringBuilderCache) instead of using StringWriter.

This could be reworked to share an implementation, without having to switch back to the desktop implementation.

Something like...

public static string HtmlEncode(string value) { ... StringBuilder sb = StringBuilderCache.Acquire(value.Length); HtmlEncode(value, index, sb, (w, s) => w.Append(s), (w, c) => w.Append(c)); return StringBuilderCache.GetStringAndRelease(sb); } public static void HtmlEncode(string value, TextWriter output) { ... HtmlEncode(value, index, output, (w, s) => w.Write(s), (w, c) => w.Write(c)); } private static unsafe void HtmlEncode<TWriter>(string value, int index, TWriter writer, Action<TWriter, string> writeString, Action<TWriter, char> writeChar) { ... // Use writeString(writer, "whatever") or writeChar(writer, 'w') }

Of course, this could be done subsequently in a separate PR.

This approach works. @safern and I discussed this before the PR went up.

The downside seems to be that it would make the existing code slower and would introduce a couple of allocations for the 2 lambdas.

would introduce a couple of allocations for the 2 lambdas.

The compiler will lazily allocate and cache/reuse the lambda delegate instances.

This would be better than the approach we discussed earlier, right? @AlexGhiondea

Just to give you a little bit of context @justinvp, we were thinking of creating an internal abstract class WriterBaseClass (this is just a random name), and making two other internal classes that would inherit from this one, those to classes would be:

SBWrapperand TextWriterWrapperwhere them would override virtual write(char)and write(string) methods from WriterBaseClassthat would write either to the StringBuilderor the TextWriter.

SBWrapper would override ToString and call StringBuilderCache.GetStringAndRelease(sb).

But I think your approach works better cause this one would have extra allocations as well and would have to make virtual calls.

By the way, I noticed the same problem being solved using delegates in a recent PR:

corefx/src/System.Runtime.Extensions/src/System/Security/SecurityElement.cs

Lines 546 to 560 in 768e18c

public override string ToString()

{

StringBuilder sb = new StringBuilder();

ToString("", sb, (obj, str) => ((StringBuilder)obj).Append(str));

return sb.ToString();

}

internal void ToWriter(StreamWriter writer)

{

ToString("", writer, (obj, str) => ((StreamWriter)obj).Write(str));

}

private void ToString(string indent, object obj, Action<object, string> write)

@safern I think they are just as good. Splitting into base classes makes it easier if you have more than 2 methods you would need to wrap. Otherwise you end up with a lot of lambdas you need to write :).

There should not be a lot of performance difference between calling via a delegate and calling via callvirt.

And if we don't think the original code is used on the hot path, then this becomes a question of what is the nicest way to write the code and @justinvp came up with an elegant one.

Ok I see! Thanks for explaining :) @AlexGhiondea
Then I'll go for @justinvp solution!

Will update this PR.

@justinvp Your solution seems like it would be slower for the main path, since an extra function call would be added for every char/string appended. Since it's more common to call the overload that returns a string rather than one that writes it to a TextWriter, it seems like what's currently checked in is the best solution.

jkotas · 2016-11-22T16:41:34Z

the nicest way to write the code

The nicest (=most compact) way to write the code (using what's available) is what @safern has done.

There should not be a lot of performance difference between calling via a delegate and calling via callvirt.

If you want to make this fast, you do not want to do either of these. Both delegate call or virtual call per character sucks performance wise.

If you want to make this fast, I believe that the best way is to introduce a new helper class to building strings that gets the underlying storage from ArrayPool. This may actually improve the string case as well in some cases because the cached StringBuilder is only up to 256 characters, and there is a good chance that encoded URLs length will be beyond this limit.

We may want to eventually get rid StringBuilderCache completely, and just switch everything to PooledStringBuilder. It has number of better characteristics compared to StringBuilderCache: works for any lengths; the buffer cache is central - there is no one StringBuilder cached per thread per each .dll.

Something like:

private struct PooledStringBuilder
{
    private char _buffer;
    private int _length = 0;

    PooledBuilder(int estimatedSize)
    {
        _buffer = ArrayPool<char>.Shared.Rent(estimatedSize);
    }

    public void Relese()
    {
        if (_buffer != null)
        {
            ArrayPool<char>.Shared.Return(_buffer);
            _buffer = null;
        }
    }

    public byte[] UnderlyingArray => _buffer;
    public int Length => _length;

    public string GetString()
    {
        return new string(_buffer, 0, _length);
    }

    public string GetStringAndRelease(sb)
    {
        string ret = GetString();
        Relese();
        return ret;
    }

    public void Append(char c)
    {
        Ensure(1);
        _buffer[_length++] = c;
    }

    public void Append(string s)
    {
        Ensure(s.Length);
        for (int i = 0; i < s.Length; i++)
             _buffer[_length++] = s[i];
    }

    private void Ensure(int extraSpace)
    {
        if ((uint)(_length + extraSpace) > (uint)_buffer.Length)
            Grow(extraSpace);
    }

    private void Grow(int extraSpace)
    {
        byte[] oldBuffer = _buffer;
        int newSize = Math.Max(2 * oldBuffer.Length, _length + extraSpace);
        byte[] newBuffer = ArrayPool<char>.Shared.Rent(newSize);
        Buffer.BlockCopy(oldBuffer, 0, newBuffer, 0, _length);
        _buffer = newBuffer;
        ArrayPool<char>.Shared.Return(oldBuffer);
    }
}

...
    PooledStringBuilder sb = new PooledStringBuilder(2 * value.Length);
    HtmlEncode(value, index, sb);
    return sb.GetStringAndRelease(sb);
...

...
    PooledStringBuilder sb = new PooledStringBuilder(2 * value.Length);
    HtmlEncode(value, index, sb);
    output.Write(sb.UnderlyingArray, 0, sb.Length);
    sb.Relese();
...

justinvp · 2016-11-22T17:09:45Z

If you want to make this fast, I believe that the best way is to introduce a new helper class to building strings that gets the underlying storage from ArrayPool.

Nice!

danmoseley · 2016-11-22T17:10:13Z

I'm guessing the STringBuilder overload is more important than the TextWriter overload? My preference is to submit the way @safern already did, which does not affect the existing code. Open an issue for someone to optimize it later probably along the lines of Jan's recommendation. It is reasonable for now to check it in-as is.

jkotas · 2016-11-22T17:20:07Z

submit the way @safern already did

Agree.

justinvp · 2016-11-22T17:30:28Z

src/System.Runtime.Extensions/tests/System/Net/WebUtility.netstandard1.7.cs

+{
+    public partial class WebUtilityTests
+    {
+    	[Theory]


Nit: Indentation looks off here. Looks like there is a tab that should be replaced with spaces.

safern · 2016-11-22T19:48:50Z

I merged with the first approach as @danmosemsft said, and will go ahead and open a new issue pointing to this PR in order to optimize this suggesting to use what @jkotas suggested.

Thanks for the reviews guys :)

@AlexGhiondea @jkotas @justinvp @danmosemsft

* Port System.Net.WebUtility missing members Commit migrated from dotnet/corefx@3cb0550

dnfclas added the cla-already-signed label Nov 22, 2016

danmoseley approved these changes Nov 22, 2016

View reviewed changes

justinvp reviewed Nov 22, 2016

View reviewed changes

safern self-assigned this Nov 22, 2016

safern added this to the 1.2.0 milestone Nov 22, 2016

safern added area-System.Net netstandard API labels Nov 22, 2016

jkotas approved these changes Nov 22, 2016

View reviewed changes

justinvp reviewed Nov 22, 2016

View reviewed changes

AlexGhiondea approved these changes Nov 22, 2016

View reviewed changes

safern added 4 commits November 22, 2016 09:51

Port WebUtility missing members

a9bc38d

Port WebUtility missing html encoding and decoding members

2edb129

Nit fix

411b2a1

PR feedback

0752110

safern force-pushed the WebUtility branch from fc2dfdf to 0752110 Compare November 22, 2016 18:14

safern merged commit 3cb0550 into dotnet:master Nov 22, 2016

safern deleted the WebUtility branch November 22, 2016 19:46

benjamin-hodgson mentioned this pull request Feb 19, 2018

Optimise HtmlEncode and HtmlDecode to produce less garbage #27250

Merged

safern mentioned this pull request Jan 31, 2020

Optimize WebUtility.HtmlEncode and WebUtility.HtmlDecode dotnet/runtime#19445

Closed

AlexRadch mentioned this pull request Jan 31, 2020

Consider pooling buffer within StringBuilder dotnet/runtime#19553

Closed

safern mentioned this pull request Jan 31, 2020

Improve Uri.UnescapeDataString codepath -> UriHelper.UnescapeString dotnet/runtime#20177

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Port System.Net.WebUtility missing members (dotnet/corefx#13875)

d8981e9

* Port System.Net.WebUtility missing members Commit migrated from dotnet/corefx@3cb0550

	public override string ToString()
	{
	StringBuilder sb = new StringBuilder();

	ToString("", sb, (obj, str) => ((StringBuilder)obj).Append(str));

	return sb.ToString();
	}

	internal void ToWriter(StreamWriter writer)
	{
	ToString("", writer, (obj, str) => ((StreamWriter)obj).Write(str));
	}

	private void ToString(string indent, object obj, Action<object, string> write)

Conversation

safern commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinvp commented Nov 22, 2016

Uh oh!

danmoseley commented Nov 22, 2016

Uh oh!

jkotas commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

safern commented Nov 22, 2016 •

edited

Loading

justinvp Nov 22, 2016 •

edited

Loading

jkotas commented Nov 22, 2016 •

edited

Loading