When encoder is null, use JavaScriptEncoder.Default to check for NeedsEscaping

[ahsonkhan]

See related PRs #41845, #41933

Now that the built-in JavaScriptEncoder implementations have been optimized, S.T.Json can leverage those rather than having its own (duplicate) logic. This is true, at least for the NeedsEscaping check.

This PR effectively reverts the optimizations made in #41845 since those went down to S.T.E.W.

There is no behavioral change here.

Ideally, we'd get rid of the encoding logic as well, when encoder == null, but the S.T.E.W implementation needs to be made faster first (for the common cases) to avoid the regression.

Something like the following change (which treats null as JavaScriptEncoder.Deafult across the board)
ahsonkhan@ced1ee0

corefx/src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs

Lines 301 to 330 in 623703d

	else
	{
	// For performance when no encoder is specified, perform escaping here for Ascii and on the
	// first occurrence of a non-Ascii character, then call into the default encoder.
	while (indexOfFirstByteToEscape < value.Length)
	{
	byte val = value[indexOfFirstByteToEscape];
	if (IsAsciiValue(val))
	{
	if (NeedsEscaping(val))
	{
	EscapeNextBytes(val, destination, ref written);
	indexOfFirstByteToEscape++;
	}
	else
	{
	destination[written] = val;
	written++;
	indexOfFirstByteToEscape++;
	}
	}
	else
	{
	// Fall back to default encoder.
	destination = destination.Slice(written);
	value = value.Slice(indexOfFirstByteToEscape);
	EscapeString(value, destination, JavaScriptEncoder.Default, ref written);
	break;
	}
	}

cc @steveharter, @GrabYourPitchforks, @gfoidl

[scalablecory]

I assume the SIMD code was moved into the default encoder? lgtm

[scalablecory]

could be:

(encoder ?? JavaScriptEncoder.Default).FindFirstCharacterToEncodeUtf8(value)

[ahsonkhan]

Good idea. This also produces smallest asm.

[scalablecory]

What is the goto getting us here now?

[ahsonkhan]

Not much perf benefit on its own, but we do produce smaller disassembly without having multiple return points in a fixed block, which could help with inlining.

[scalablecory]

really? it seems like the difference would be a jmp vs a ret; is there some stack space cleanup?

[ahsonkhan]

Never mind, re-checked the assembly diff, and removing the goto is better in terms of asm size.

// Some implementations of JavascriptEncoder.FindFirstCharacterToEncode may not accept
// null pointers and gaurd against that. Hence, check up-front and fall down to return -1.
if (value.IsEmpty)
{
    return -1; // All characters are allowed.
}

fixed (char* ptr = value)
{
    return (encoder ?? JavaScriptEncoder.Default).FindFirstCharacterToEncode(ptr, value.Length);
}

versus

public static unsafe int NeedsEscaping(ReadOnlySpan<char> value, JavaScriptEncoder encoder)
{
    fixed (char* ptr = value)
    {
        int idx = 0;

        // Some implementations of JavascriptEncoder.FindFirstCharacterToEncode may not accept
        // null pointers and gaurd against that. Hence, check up-front and fall down to return -1.
        if (value.IsEmpty)
        {
            idx = -1; // All characters are allowed.
            goto Return;
        }

        idx = (encoder ?? JavaScriptEncoder.Default).FindFirstCharacterToEncode(ptr, value.Length);

    Return:
        return idx;
    }
}

See diff (3 fewer instructions total):
https://www.diffchecker.com/uqTkCcBl

[ahsonkhan]

I assume the SIMD code was moved into the default encoder?

Yep.

[ahsonkhan]

Unrelated test failure: https://github.com/dotnet/corefx/issues/41968

[gfoidl]

when !BUILDING_INBOX_LIBRARY

Put into #if !BUILDING_INBOX_LIBRARY to make this (code-) explicit?

[ahsonkhan]

Good idea. I will do this separately (for master) since it is primarily a cosmetic change and I want to cherry-pick this change for 3.1 (was waiting on CI to be green for a while :p).