Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add readonly dictionary wrapper that removes enumerator alloc #6163

Merged
merged 3 commits into from Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
156 changes: 156 additions & 0 deletions osu.Framework.Benchmarks/BenchmarkSlimReadOnlyDictionary.cs
@@ -0,0 +1,156 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Collections.Generic;
using System.Collections.ObjectModel;
using BenchmarkDotNet.Attributes;
using osu.Framework.Extensions.ListExtensions;

namespace osu.Framework.Benchmarks
{
[MemoryDiagnoser]
public class BenchmarkSlimReadOnlyDictionary
{
private readonly Dictionary<int, int> dictionary = new Dictionary<int, int>();

[GlobalSetup]
public void GlobalSetup()
{
int[] values = { 0, 1, 2, 3, 4, 5, 3, 2, 3, 1, 4, 5, -1 };
for (int i = 0; i < values.Length; i++)
dictionary[i] = values[i];
}

[Benchmark]
public int Dictionary()
{
int sum = 0;

for (int i = 0; i < 1000; i++)
{
foreach ((_, int v) in dictionary)
sum += v;
}

return sum;
}

[Benchmark]
public int DictionaryAsReadOnly()
{
int sum = 0;

ReadOnlyDictionary<int, int> readOnlyDictionary = new ReadOnlyDictionary<int, int>(dictionary);
bdach marked this conversation as resolved.
Show resolved Hide resolved

for (int i = 0; i < 1000; i++)
{
foreach ((_, int v) in readOnlyDictionary)
sum += v;
}

return sum;
}

[Benchmark]
public int DictionaryAsSlimReadOnly()
{
int sum = 0;

for (int i = 0; i < 1000; i++)
{
foreach ((_, int v) in dictionary.AsSlimReadOnly())
sum += v;
}

return sum;
}

[Benchmark]
public int Keys()
{
int sum = 0;

for (int i = 0; i < 1000; i++)
{
foreach (int v in dictionary.Keys)
sum += v;
}

return sum;
}

[Benchmark]
public int KeysAsReadOnly()
{
int sum = 0;

ReadOnlyDictionary<int, int> readOnlyDictionary = new ReadOnlyDictionary<int, int>(dictionary);
bdach marked this conversation as resolved.
Show resolved Hide resolved

for (int i = 0; i < 1000; i++)
{
foreach (int v in readOnlyDictionary.Keys)
sum += v;
}

return sum;
}

[Benchmark]
public int KeysAsSlimReadOnly()
{
int sum = 0;

for (int i = 0; i < 1000; i++)
{
foreach (int v in dictionary.AsSlimReadOnly().Keys)
sum += v;
}

return sum;
}

[Benchmark]
public int Values()
{
int sum = 0;

for (int i = 0; i < 1000; i++)
{
foreach (int v in dictionary.Values)
sum += v;
}

return sum;
}

[Benchmark]
public int ValuesAsReadOnly()
{
int sum = 0;

ReadOnlyDictionary<int, int> readOnlyDictionary = new ReadOnlyDictionary<int, int>(dictionary);

for (int i = 0; i < 1000; i++)
{
foreach (int v in readOnlyDictionary.Values)
sum += v;
}

return sum;
}

[Benchmark]
public int ValuesAsSlimReadOnly()
{
int sum = 0;

for (int i = 0; i < 1000; i++)
{
foreach (int v in dictionary.AsSlimReadOnly().Values)
sum += v;
}

return sum;
}
}
}
Expand Up @@ -8,7 +8,7 @@
namespace osu.Framework.Benchmarks
{
[MemoryDiagnoser]
public class BenchmarkSlimReadOnlyCollection
public class BenchmarkSlimReadOnlyList
{
private readonly List<int> list = new List<int> { 0, 1, 2, 3, 4, 5, 3, 2, 3, 1, 4, 5, -1 };

Expand Down
12 changes: 12 additions & 0 deletions osu.Framework/Extensions/ListExtensions/ListExtensions.cs
Expand Up @@ -17,5 +17,17 @@ public static class ListExtensions
/// <returns>The read-only view.</returns>
public static SlimReadOnlyListWrapper<T> AsSlimReadOnly<T>(this List<T> list)
=> new SlimReadOnlyListWrapper<T>(list);

/// <summary>
/// Creates a new read-only view into a <see cref="Dictionary{TKey, TValue}"/>.
/// </summary>
/// <remarks>Enumeration does not allocate the enumerator.</remarks>
/// <param name="dictionary">The dictionary to create the view of.</param>
/// <typeparam name="TKey">The type of keys in the dictionary.</typeparam>
/// <typeparam name="TValue">The type of values in the dictionary.</typeparam>
/// <returns>The read-only view.</returns>
public static SlimReadOnlyDictionaryWrapper<TKey, TValue> AsSlimReadOnly<TKey, TValue>(this Dictionary<TKey, TValue> dictionary)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the namespace of this is a little unfortunate but probably not worth incurring a source breaking change for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could split it out into a DictionaryExtensions, but it felt like too much. If you'd feel better with that then let me know and I'll do it.

where TKey : notnull
=> new SlimReadOnlyDictionaryWrapper<TKey, TValue>(dictionary);
}
}
53 changes: 53 additions & 0 deletions osu.Framework/Lists/SlimReadOnlyDictionaryWrapper.cs
@@ -0,0 +1,53 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;

namespace osu.Framework.Lists
{
public readonly struct SlimReadOnlyDictionaryWrapper<TKey, TValue> : IReadOnlyDictionary<TKey, TValue>
where TKey : notnull
{
private readonly Dictionary<TKey, TValue> dict;

public SlimReadOnlyDictionaryWrapper(Dictionary<TKey, TValue> dict)
{
this.dict = dict;
}

IEnumerator<KeyValuePair<TKey, TValue>> IEnumerable<KeyValuePair<TKey, TValue>>.GetEnumerator()
=> dict.GetEnumerator();
Comment on lines +20 to +21
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the non-generic IEnumerator one) still boxes, but I guess SlimReadOnlyListWrapper does the same so maybe fine.

Copy link
Contributor Author

@smoogipoo smoogipoo Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, and is the way you're supposed to implement this. The only relevant case is foreach ((SlimReadOnlyDictionary<>)dict), if you go via the interface then you're SOL no matter what.


public Dictionary<TKey, TValue>.Enumerator GetEnumerator()
=> dict.GetEnumerator();

public Dictionary<TKey, TValue>.KeyCollection Keys
=> dict.Keys;

public Dictionary<TKey, TValue>.ValueCollection Values
=> dict.Values;

public bool ContainsKey(TKey key)
=> dict.ContainsKey(key);

public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value)
=> dict.TryGetValue(key, out value);

public TValue this[TKey key]
=> dict[key];

IEnumerable<TKey> IReadOnlyDictionary<TKey, TValue>.Keys
=> dict.Keys;

IEnumerable<TValue> IReadOnlyDictionary<TKey, TValue>.Values
=> dict.Values;

IEnumerator IEnumerable.GetEnumerator()
=> GetEnumerator();

public int Count
=> dict.Count;
}
}