Support IDictionary<string,TValue> without IDictionaryEnumerator#41903
Conversation
|
Can you test and see if it also fixes |
layomia
left a comment
layomia
left a comment
There was a problem hiding this comment.
Some comments, pending the sync with master.
| ClassType == ClassType.IDictionaryConstructible); | ||
|
|
||
| if (writeStackFrame.CollectionEnumerator is IDictionaryEnumerator iDictionaryEnumerator) | ||
| { |
There was a problem hiding this comment.
Would it be beneficial to perform this check at the calling site of this method, since we don't need the generic support? That way we don't need to call into the JsonPropertyInfo.
On the other hand, would performing the generic check done in DictionaryValuePropertyPolicy.GetDictionaryKeyAndValueFromGenericDictionary() first (before checking for IDictionaryEnumerator) have a perf benefit (by avoiding boxing) for types whose .GetEnumerator() method implements both IDictionaryEnumerator and IEnumerator<KeyValuePair<,>> (e.g. Dictionary<,>)?
There was a problem hiding this comment.
It is called from more than one location, so keeping it in the helper method is a bit cleaner.
On the generic check, I think it is better to check for non-generic first in this case as the generic check will unnecessary create a "policy property" (which is expensive) in the non-generic cases.
Also, as a separate PR I'm working on avoid boxing in general for this case for perf...
|
|
||
| if (elementClassInfo.ClassType == ClassType.Value) | ||
| { | ||
| elementClassInfo.PolicyProperty.WriteDictionary(ref state, writer); |
There was a problem hiding this comment.
Since we might have the value here from the polymorphic check above, should we have logic here to see if we do, and just write the key and value here, skipping all the logic that will happen in (and on the path to) WriteDictionary<TProperty>() below?
There was a problem hiding this comment.
Yes passing the key\value down is possible here but requires a new helper method that would essentially copy most of the code\flow in WriteDictionary in order to bounce from non-generic to generic. Plus add associated tests.
Since the original code also obtained the key and value before calling WriteDictionary, which didn't change here, I'll include this optimization in the other PR mentioned earlier for boxing. Thanks
I replied to that PR for a repro. I couldn't repro the original issue, but based on the exception it appears it would be fixed by this PR. |
@steveharter - can you use this commit to verify |
Support IDictionary<string,TValue> without IDictionaryEnumerator Commit migrated from dotnet/corefx@d3c2984
3 participants
Fixes https://github.com/dotnet/corefx/issues/41329. I was unable to verify against the linked repro because
SqlMapper.DapperRowand.DapperTableare not public members.Originally this was going to also have a perf fix that removes boxing of the dictionary enumerator as well, but I decided to just have the raw fix that enables
IDictionary<TKey, TValue>-derived dictionaries to work properly.This may have some conflicts with #41482
This is expected to be ported to 3.1.