Skip to content

Conversation

@h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Aug 11, 2024

Description

Replaces two boxing Hashtable collections with generic Dictionary<K, V>, improving performance and decreasing allocations.

  • There are no locks but I do not believe there are any thread-safety concerns as the collections are only set once per my code inspection and testing it with few sample apps, plus if it was a concern, even Hashtable would need lock for proper functionality in this scenario.
  • Worst case, we could swap it to ConcurrentDictionary<K, V> which would still work out very well over boxing Hashtable in read-only context.
  • ZRange is now a readonly struct, further decreasing memory footprint of the underlying collection.
  • As AdornerPresentationContext is a derived class, I've sealed it.

Base difference between boxing Hashtable and Dictionary<K, V>

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 339.5 ns 6.56 ns 5.81 ns 0.1044 2,756 B 1752 B
PR__EDIT 202.3 ns 4.02 ns 6.38 ns 0.0534 1,301 B 896 B

More of a real use-case in AdornerPresentationContext

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 1,269.0 ns 20.86 ns 18.49 ns 0.3757 2,775 B 6312 B
PR__EDIT 773.8 ns 5.49 ns 4.58 ns 0.0534 1,283 B 896 B
Benchmark code
[Benchmark]
public void Original()
{
    Hashtable ZRanges = new Hashtable();

    for (int i = 0; i < 10; i++)
        BenchV1(i, ZRanges);

    //200 is used for the second benchmark instead of ZRanges!.Count
    for (int i = 0; i < ZRanges!.Count; i++)
    {
        ZRange range = (ZRange)ZRanges[i];
        if (range != null)
            EmptyMethod(range);
    }
}

[Benchmark]
public void PR__EDIT()
{
    Dictionary<int, ZRangeStruct> ZRanges = new();

    for (int i = 0; i < 10; i++)
        BenchV2(i, ZRanges);

    //200 is used for the second benchmark instead of ZRanges!.Count
    for (int i = 0; i < ZRanges!.Count; i++)
    {
        if (ZRanges.TryGetValue(i, out ZRangeStruct range))
            EmptyMethod2(ref range);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int EmptyMethod(ZRange zRange) => zRange.Min;

//We use ref to prevent copy that doesn't happen in actual code
[MethodImpl(MethodImplOptions.NoInlining)]
private static int EmptyMethod2(ref ZRangeStruct zRange) => zRange.Min;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void BenchV1(int level, Hashtable ZRanges)
{
    if (ZRanges[level] == null)
        ZRanges.Add(level, new ZRange(1, 2));
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void BenchV2(int level, Dictionary<int, ZRangeStruct> ZRanges)
{
    if (!ZRanges.ContainsKey(level))
        ZRanges.Add(level, new ZRangeStruct(1, 2));
}

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build, sample apps run.

Risk

There is a small concern as per my first point, however again, very unlikely given how the code is written.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners August 11, 2024 12:37
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Aug 11, 2024
@harshit7962
Copy link
Member

@h3xds1nz Thank you for your contributions.

@harshit7962 harshit7962 merged commit 1a9d0c3 into dotnet:main Oct 1, 2024
@h3xds1nz
Copy link
Member Author

h3xds1nz commented Oct 1, 2024

@harshit7962 Thank you for including them.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants