[CSHARP-2556] Reduce memory allocations in BsonSerializerRegistry.GetSerializer() Created: 20/Mar/19 Updated: 08/Jun/23 |
|
| Status: | Backlog |
| Project: | C# Driver |
| Component/s: | Performance, Serialization |
| Affects Version/s: | 2.8.0 |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Daniel Hegener | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||
| Issue Links: |
|
||||
| Backwards Compatibility: | Fully Compatible | ||||
| Description |
|
The BsonSerializerRegistry.GetSerializer() gets called repeatedly on the hot path during serialization/deserialization. It allocates memory due to an implicit closure of the current instance variable ("this").
This allocation can be avoided. For the versions of the .NET frameworks which we are dealing with (.NET 4.5.2/.NET Standard 1.5) the following approach would work: https://github.com/dotnet/corefx/issues/394 From .NET 4.7.2 onwards it is possible to simply switch to a special overload of the ConcurrentDictionary.GetOrAdd() method which accepts a factory argument as described here https://github.com/dotnet/corefx/pull/1783 and documented here (https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2.getoradd?view=netframework-4.7.2#System_Collections_Concurrent_ConcurrentDictionary_2_GetOrAdd__1__0_System_Func__0___0__1____0_) |
| Comments |
| Comment by Daniel Hegener [ 20/Jun/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I ran some basic benchmark (https://gist.github.com/dnickless/7dcd4f8d488c791a478f76e9acba4be2) using various configurations and a .NET 4.7.2 based implementation where I also moved the reflection based checks inside the factory:
Here are the results: WITHOUT changes, original version:
WITH changes
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ian Whalen (Inactive) [ 25/Mar/19 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dnickless thanks for the report - do you have any idea of the perf impact of the proposed changes here? any rough tests you can share, etc? |