Allow deserialization when properties case doesn't match #251
Conversation
torbacz
changed the title
nfbot
changed the title
torbacz
changed the title
…est - inconsistent hour throw. TimeSpan.ToString returns 24:00:00.
|
Each benchmark was 2500 times resolving the same property, each test class contained 5 fields/properties and the last one was the one which was resolved. [IterationCount(10)]
public class ResolverFieldBenchmark
{
private class ClassWithField
{
public string TestField1;
public string TestField2;
public string TestField3;
public string TestField4;
public string TestField5;
}
private CaseSensitiveResolver caseSensitiveResolver;
private CaseInsensitiveResolver caseInsensitiveResolver;
private Type typeToCheck;
private const short ResolvingCount = 2500;
[Setup]
public void SetUp()
{
caseSensitiveResolver = new CaseSensitiveResolver();
caseInsensitiveResolver = new CaseInsensitiveResolver();
typeToCheck = typeof(ClassWithField);
}
[Benchmark]
[Baseline]
public void CaseSensitive_FieldResolve()
{
for (int i = 0; i < ResolvingCount; i++)
{
var field = caseSensitiveResolver.GetResolver(nameof(ClassWithField.TestField5), typeToCheck);
}
}
[Benchmark]
public void CaseInsensitive_FieldResolve()
{
for (int i = 0; i < ResolvingCount; i++)
{
var field = caseInsensitiveResolver.GetResolver(nameof(ClassWithField.TestField5), typeToCheck);
}
}
}
[IterationCount(10)]
public class ResolverPropertyBenchmark
{
private class ClassWithProperty
{
public string TestField1 { get; set; }
public string TestField2 { get; set; }
public string TestField3 { get; set; }
public string TestField4 { get; set; }
public string TestField5 { get; set; }
}
private CaseSensitiveResolver caseSensitiveResolver;
private CaseInsensitiveResolver caseInsensitiveResolver;
private Type typeToCheck;
private const short ResolvingCount = 2500;
[Setup]
public void SetUp()
{
caseSensitiveResolver = new CaseSensitiveResolver();
caseInsensitiveResolver = new CaseInsensitiveResolver();
typeToCheck = typeof(ClassWithProperty);
}
[Benchmark]
[Baseline]
public void CaseSensitive_PropertyResolve()
{
for (int i = 0; i < ResolvingCount; i++)
{
var field = caseSensitiveResolver.GetResolver(nameof(ClassWithProperty.TestField5), typeToCheck);
}
}
[Benchmark]
public void CaseInensitive_PropertyResolve()
{
for (int i = 0; i < ResolvingCount; i++)
{
var field = caseInsensitiveResolver.GetResolver(nameof(ClassWithProperty.TestField5), typeToCheck);
}
}
}Like i thought, current implementation for insensitive members resolving is much slower. It's none default one, so it has no impact on standard mode. |
That was indeed expected :-) Bad can we make a simple test to find a property and if nothing found then try the insensitive case? So we would go the normal way and deserialize what can be. Once done, just checking is anything is left from both the json token and the class. If something left, let's try the insensitive case. We'll still loose few cycles because of the minimum check of what has been deserialized and what's left but that's it for the main part. And then we won't lose as much on what's left because we know what's left! What do you think? |
|
It would mean:
I would rather go with similar approach as current, but instead of having two "Resolvers" just merge them into one. In short, if the "case sensitive" can't find member, then call "case insensitive". If "case insensitive" can't find member - then throw exception. It would also mean removing few lines of code and classes. |
|
I'm not seeing an easy alternative to tackle this. Any of the approaches that are possible with what we have available will incurr in a performance penalty. Questions:
|
Yes, we can definitely add an option for that. That will allow to add options for other scenarios like in the larger framework. And we then should be very very very clear on the performance impact. Still if we add the option, I do think we should try to minimize the perf impact. But 100% agree, there will be one! |
|
@josesimoes One more proposition from me. If we can't find property with match case, we are calling case insensitive methods. If case insensitive fails, then throw exception. |
We should not throw an exception, a property can be absent and kept with default value or null. I'm fine with the following logic:
I would still love to see the impact on the performance, but with those options, I'm quite sure it's very minimum and very acceptable.
Can't wait :-D |
|
I'll look into adding support for case sensitive comparison in mscorlib. Just can't promise it for tomorrow... |
|
@Ellerbach @josesimoes |
Thanks. I have to say I like the logic. And for the settings, the throw if member does not exist should be false by default here https://github.com/torbacz/nanoFramework.Json/blob/cc521313b5869e784916ed4d3a827d69a6e37598/nanoFramework.Json/Configuration/Settings.cs#L18 So this will still involve a little bit of performance issue compare to the "past" use case where we had nothing at all. But with the recent improvements, we can for sure lose a little bit to be able to get this very nice addition. What I also like is the possibility to have your own resolver. That can be very handy to map some enums or strings or whatever. |
|
New benchmark results Legend:
I'm a little bit worried about about deserialization for "NestedClasss" and "ComplexArrayObject". I want to investigate it before merging. Edit: Also, explicitly pointed out ThrowExceptionWhenPropertyNotFound is false by default. |
|
@torbacz this is looking good! I share your worries. Those are heavily used cases for json payloads... |
|
@josesimoes I found it, the "problem" is in StringConverter, when I've changed it to "just return input object" the performance was pretty much the same. But it needs to go through all the logic to escape all required characters in string object. In my option we should:
Then someone (probably I'll take it) can work on further cleanup/performance in JSON. |
|
@torbacz nice finding!! 👏🏻 persistence pays off!! 😉 There's a project for the json library. Please add any tasks and to-do's there. We can move those to issues in order to track progress as needed. I would suggest that we discuss this on Discord. It's a more suitable and streamlined experience. (we can add the URL for the thread here for reference) |
|
@josesimoes Are we ready to merge this one? |
|
@torbacz I'm good with these changes. OK for discussing on Discord. 👍🏻 @Ellerbach any other take on this? |
I'm good! Let's merge it! |
Description
NOTE: Benchmarks without IntArray and ShortArray due to possible bug.
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: