Fix JsonNode performance regression

[eiriktsarpalis]

Fixes dotnet/perf-autofiling-issues#36936 (comment)

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

[stephentoub]

The change looks good, but I'm a little surprised it caused such regressions. It looks like CreateFromElement is also using this constructor but before that is checking ValueKind against the same three cases above. Those branches were enough to cause significant regressions? This must be a super hot path?

[eiriktsarpalis]

It's in the hot path for the lazy conversion of JsonArray and JsonObject from JsonElement backed to List and dictionary backed, which is what regressed by this change.

[eiriktsarpalis]

I wonder if the ref readonly parameter in the removed method contributed to the regression somehow.

[andrewjsaid]

My guess is that it calls element.ValueKind twice, which resolves as follows

public JsonValueKind ValueKind => TokenType.ToValueKind();`

private JsonTokenType TokenType => _parent?.GetJsonTokenType(_idx) ?? JsonTokenType.None

internal JsonTokenType GetJsonTokenType(int index)
{
    CheckNotDisposed();

    return _parsedData.GetJsonTokenType(index);
}

internal JsonTokenType GetJsonTokenType(int index)
{
    AssertValidIndex(index);
    uint union = MemoryMarshal.Read<uint>(_data.AsSpan(index + NumberOfRowsOffset));

    return (JsonTokenType)(union >> 28);
}

plus a small cost to run the switch statement in internal static JsonValueKind ToValueKind(this JsonTokenType tokenType)

Which is quite a lot of indirection. Possibly changing CreateFromElement to create a local JsonValueKind kind = element.ValueKind; to avoid running the above twice? But still maybe running it once is even a lot here.

[andrewjsaid]

Created a PR here #104108