Moving inline map creation back to inside type map resolving #2403
Conversation
| private static readonly SourceMember[] Empty = new SourceMember[0]; | ||
| private readonly Dictionary<TypeDetails, SourceMember[]> _allSourceMembers = new Dictionary<TypeDetails, SourceMember[]>(); | ||
| private LockingConcurrentDictionary<TypeDetails, SourceMember[]> _allSourceMembers | ||
| = new LockingConcurrentDictionary<TypeDetails, SourceMember[]>(_ => Empty); |
There was a problem hiding this comment.
I don't think we should lock here. It's difficult to write correct thread safe code in a lot of places. I think we should lock when calling CreateMap.
There was a problem hiding this comment.
We already lock there but this is another place that's a static cache. Another option would be just to get rid of the cache.
There was a problem hiding this comment.
The idea was to not look for the attribute every time it tries to match the members to map. So it will be slower without the cache. But my point point is more general. A coarser grained lock would make the code easier to write and understand. That's why I added a global lock before each CreateMap. So we don't have to worry about concurrency at every point.
| && !ExcludedTypes.Contains(initialTypes.SourceType) | ||
| && !ExcludedTypes.Contains(initialTypes.DestinationType)) | ||
| { | ||
| return Configuration.CreateInlineMap(_typeMapRegistry, initialTypes); |
There was a problem hiding this comment.
I would lock(this) here, so all CreateMap calls are serialized, closed generics, conventions and inline.
There was a problem hiding this comment.
Sounds good. I got rid of the cache, it's just not obvious.
There was a problem hiding this comment.
Ah but that's the value factory for the locking concurrent dictionary. The lock is already there.
There was a problem hiding this comment.
Yes, I know there's a lock for the type pair, I'm just not sure that's enough. I'll have to think about it some more.
There was a problem hiding this comment.
I don't think it's enough. A GetType map for some other types might call CreateMap for these initialTypes. And then you have concurrent CreateMap calls again.
There was a problem hiding this comment.
Another thing, I don't think we need a Lazy here. A regular bool? should do. It's a local, only one thread will access it and the mappers collection is immutable.
There was a problem hiding this comment.
Ah you're right, I missed the other locks.
There was a problem hiding this comment.
I use the hasMapper in two places though? That would look a little odd, I might need a func or something.
There was a problem hiding this comment.
I don't have a problem with
var hasMapper = (FindMapper(initialTypes) != null);
Or simply calling it twice as before.
|
Good to go? |
|
It's not a big deal, but... |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
2 participants
Fixes #2394