Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add support for ISet<T>#38210

Closed
layomia wants to merge 4 commits intodotnet:masterfrom
layomia:iset
Closed

Add support for ISet<T>#38210
layomia wants to merge 4 commits intodotnet:masterfrom
layomia:iset

Conversation

@layomia
Copy link
Copy Markdown
Contributor

@layomia layomia commented Jun 4, 2019

The approach is to treat it like HashSet, similar to how we treat IDictionary<string, TValue> and IReadOnlyDictionary<string, TValue> as Dictionary<string, TValue>.

Perf

We might see some perf gain by writing a special-case converter for ISet<T>. This will also help mitigate the limitation on (de)serializing ISet<ISet<T>> (and higher degrees of ISet<T> nesting) that I describe in this PR.
For now, I'd rather spend the time on getting this and other collections through (https://github.com/dotnet/corefx/issues/36643), and circle back during/following testing/validation.

ISet<T> (de)serialization

The benchmarks are being added to the perf repo: dotnet/performance#540.

Method ElementCount Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
DeserializeISet 2 2,114.5 ns 41.533 ns 44.440 ns 0.2747 - - 1160 B
SerializeISet_ToBytes 2 507.2 ns 5.404 ns 4.791 ns 0.0591 - - 248 B
DeserializeISet 50 11,712.8 ns 252.800 ns 346.035 ns 1.8616 - - 7817 B
SerializeISet_ToBytes 50 5,993.0 ns 106.807 ns 99.907 ns 0.2823 - - 1192 B
DeserializeISet 100 21,081.3 ns 124.253 ns 103.757 ns 3.6621 - - 15353 B
SerializeISet_ToBytes 100 11,535.3 ns 173.335 ns 135.328 ns 0.5188 - - 2192 B
Json.Net
Method ElementCount Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated
DeserializeISet 2 21,030.9 ns 3,649.67 ns 10,412.72 ns 17,650.0 ns - - - 2.84 KB
SerializeISet_ToBytes 2 542.6 ns 10.85 ns 23.35 ns 536.1 ns 0.3328 - - 1.36 KB
DeserializeISet 50 9,447.0 ns 188.40 ns 224.27 ns 9,434.3 ns 1.9073 - - 7.85 KB
SerializeISet_ToBytes 50 4,475.9 ns 86.77 ns 72.46 ns 4,493.8 ns 0.6943 - - 2.85 KB
DeserializeISet 100 18,215.9 ns 70.56 ns 62.55 ns 18,209.3 ns 3.3264 - - 13.7 KB
SerializeISet_ToBytes 100 8,535.8 ns 70.04 ns 65.51 ns 8,553.2 ns 1.1902 - - 4.9 KB
public class JsonDotNet_ReadGenericCollection
{
    private static readonly ISet<string> _iset = new HashSet<string>();
    private static string _jsonString;

    [Params(2, 50, 100)]
    public int ElementCount;

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < ElementCount; i++)
        {
            _iset.Add($"hello{i}");
        }

        _jsonString = System.Text.Json.Serialization.JsonSerializer.ToString(_iset);
    }

    [Benchmark]
    public ISet<string> DeserializeISet()
    {
        return JsonConvert.DeserializeObject<ISet<string>>(_jsonString);
    }

    [Benchmark]
    public string SerializeISet_ToBytes()
    {
        return JsonConvert.SerializeObject(_iset);
    }
}
Hot-path (de)serialization scenarios

Benchmarks live in the perf repo.
No regressions: hot-path benchmark results.zip

UPDATE
The limitation on (de)serializing ISet<ISet<T>> is fixed by using a runtime property type of HashSet<TDeclaredPropertyOfElement> instead of HashSet<TRuntimePropertyOfElement> for ISet<TDeclaredPropertyOfElement>

@layomia layomia requested review from ahsonkhan and steveharter June 4, 2019 15:41
@layomia layomia self-assigned this Jun 4, 2019
@layomia layomia added this to the 3.0 milestone Jun 4, 2019
@steveharter
Copy link
Copy Markdown
Contributor

On the benchmarks - Json.NET takes longer to deserialize 2 items than 50 or 100?

@layomia
Copy link
Copy Markdown
Contributor Author

layomia commented Jun 4, 2019

On the benchmarks - Json.NET takes longer to deserialize 2 items than 50 or 100?

Yes, even on repeated runs. I’m looking into why. First guess is cached reflection.

Comment thread src/System.Text.Json/tests/Serialization/Value.ReadTests.GenericCollections.cs Outdated
Comment thread src/System.Text.Json/tests/Serialization/Value.WriteTests.GenericCollections.cs Outdated
Comment thread src/System.Text.Json/tests/Serialization/PolymorphicTests.cs
@ahsonkhan
Copy link
Copy Markdown

On the benchmarks - Json.NET takes longer to deserialize 2 items than 50 or 100?

Yes, even on repeated runs. I’m looking into why. First guess is cached reflection.

Looking at the Error/StdDev, that test looks noisy. You want higher warmup count probably.

@layomia
Copy link
Copy Markdown
Contributor Author

layomia commented Jun 10, 2019

Changes here have been merged in #38319.

@layomia layomia closed this Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants