Skip to content

CSHARP-2450: Reduce locking costs in BsonSerializer #482

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 29, 2021
Merged

CSHARP-2450: Reduce locking costs in BsonSerializer #482

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e4b02e
CSHARP-2450: Improved deserialization performance by switching from H…
JamesKovacs Mar 24, 2021
440c559
Incorporated PR feedback.
JamesKovacs Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions src/MongoDB.Bson/Serialization/BsonSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -41,7 +42,8 @@ public static class BsonSerializer
private static HashSet<Type> __discriminatedTypes = new HashSet<Type>();
private static BsonSerializerRegistry __serializerRegistry;
private static TypeMappingSerializationProvider __typeMappingSerializationProvider;
private static HashSet<Type> __typesWithRegisteredKnownTypes = new HashSet<Type>();
// ConcurrentDictionary<Type, object> is being used as a concurrent set of Type. The values will always be null.
private static ConcurrentDictionary<Type, object> __typesWithRegisteredKnownTypes = new ConcurrentDictionary<Type, object>();

private static bool __useNullIdChecker = false;
private static bool __useZeroIdChecker = false;
Expand Down Expand Up @@ -679,23 +681,15 @@ public static void Serialize(
// internal static methods
internal static void EnsureKnownTypesAreRegistered(Type nominalType)
{
__configLock.EnterReadLock();
try
{
if (__typesWithRegisteredKnownTypes.Contains(nominalType))
{
return;
}
}
finally
if (__typesWithRegisteredKnownTypes.ContainsKey(nominalType))
{
__configLock.ExitReadLock();
return;
}

__configLock.EnterWriteLock();
try
{
if (!__typesWithRegisteredKnownTypes.Contains(nominalType))
if (!__typesWithRegisteredKnownTypes.ContainsKey(nominalType))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Consider using GetOrAdd instead of the if-ContainsKey-assignment sequence as this will cause two lookups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feeback!

GetOrAdd(TKey key, Func<TKey,TValue> valueFactory) internally performs multiple lookups. First to determine if the key is present and then after executing valueFactory but prior to inserting into the collection. If the value is not present in the collection, GetOrAdd will perform two lookups and the current ContainsKey/[]=value does the same. If the value is already present, both only perform a single lookup. Testing out both implementations I see the same performance numbers.

I will add a comment to __typesWithRegisteredKnownTypes[nominalType] = nominalType; noting that it must be performed as the last step to ensure that other threads don't see partially initialized types.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right about the actual lookups, of course. I was thinking about the GetHashCode() calls of which you'd be saving one but, in this case, they're not costly so let's ignore that. It's just a standard thing that I constantly tell my devs: "Always use GetOrAdd instead of the other sequence!"... ;)

In fact, I had originally been coming from suggesting the use of TryAdd() which should have saved the lookups but that didn't work as there's additional work to be done inside the if branch and it needs to happen before the dictionary insert... Anyway, that code is very ok the way it is now, I suppose.

{
// only call LookupClassMap for classes with a BsonKnownTypesAttribute
#if NET452
Expand All @@ -709,7 +703,10 @@ internal static void EnsureKnownTypesAreRegistered(Type nominalType)
LookupSerializer(nominalType);
}

__typesWithRegisteredKnownTypes.Add(nominalType);
// NOTE: The nominalType MUST be added to __typesWithRegisteredKnownTypes after all registration
// work is done to ensure that other threads don't access a partially registered nominalType
// when performing the initial check above outside the __config lock.
__typesWithRegisteredKnownTypes[nominalType] = null;
}
}
finally
Expand Down