Add support of pure IEnumerable classes (instead of ICollection)

[zachsaw]

Add support of pure IEnumerable classes (instead of ICollection)

[zachsaw]

That's true. I'll see what I can do when I have a bit of spare time. Wouldn't mind a PR though if you give it a try.

[nikolaygekht]

Good to know. Definitely, I will.

[nikolaygekht]

Ok, I have a solution in my copy of repo
https://github.com/nikolaygekht/Binaron.Serializer
The cost at this moment is more slow restoration:

Original benchmark:

|              Method |        Mean |     Error |    StdDev |
|-------------------- |------------:|----------:|----------:|
|   Binaron_Serialize |    12.91 ms |  0.174 ms |  0.163 ms |
| Binaron_Deserialize |    11.37 ms |  0.224 ms |  0.342 ms |
                           ^^^^^^^
|      Json_Serialize | 1,025.35 ms | 16.210 ms | 15.163 ms |
|    Json_Deserialize | 1,746.62 ms | 19.394 ms | 17.192 ms |

|              Method |        Mean |     Error |    StdDev |
|-------------------- |------------:|----------:|----------:|
|   Binaron_Serialize |    13.09 ms |  0.246 ms |  0.242 ms |
| Binaron_Deserialize |   97.06 ms |  0.717 ms |  0.599 ms |
                           ^^^^^^^
|      Json_Serialize | 1,012.36 ms |  9.614 ms |  8.993 ms |
|    Json_Deserialize | 1,772.35 ms | 14.614 ms | 13.670 ms |

It is inevitable for IEnumerable, but I'll check whether it could be avoided for ICollection without writing tons of "ifs"

[zachsaw]

That's 8.5x slower than original even when you're not using the feature. For this to work, we need to make sure it only pays the penalty when this particular feature is required. The data structures for caching all of the object creation should already be there - you just need to extend it to make it support this new way of object activation and population. This way, we should have no adverse effect on performance.

[nikolaygekht]

That's a good point. If you don't mind, I would like to discuss it a little bit more. That was my intent to go this way. Unfortunately, either I didn't get something or it is not possible right now. And it is true for both issues. The problem is that it adds to cache relying on a supposition that the ICollection interface is supported (and that default constructor exists in case of the other issue). The actual validation of this supposition happens only during the execution.

So, there are two ways to work here:

• Rewrite the logic of caching, making checks such as "is ICollection or IEnumerable" supported or "is default constructor available" before the appropriate reader is created and cached.
• Make execution logic different depending on the situation (the way I went). Of course, because it involves additional checks, it has its price.

I understand the logic why the first way is preferable, but I don't see how to do it without refactoring the whole caching logic, which is, unfortunately, deeply coupled with other components. So, I am open to your suggestions if you see any less invasive way to do it.

p.s. Meanwhile, I optimized the current option, it still has its price, but way less now than before. Further optimization is to duplicate a few dozen ifs in GenericReader.cs. (if (result is ICollection) ...) inside switch (TypeOf.TypeCode), but it would make it even more longer. To tell the truth, if we decide to go this way I would rather switch to emitting the IL code on the fly, it will have way less "repeating yourself" without any significant price.

|              Method |        Mean |    Error |   StdDev |
|-------------------- |------------:|---------:|---------:|
|   Binaron_Serialize |    12.96 ms | 0.188 ms | 0.176 ms |
| Binaron_Deserialize |    26.82 ms | 0.303 ms | 0.284 ms |
|      Json_Serialize | 1,007.49 ms | 3.591 ms | 2.999 ms |
|    Json_Deserialize | 1,717.02 ms | 5.006 ms | 4.181 ms |

[nikolaygekht]

Just in case: A version that does not touch the old implementation at all and provides a whole new implementation for classes that do not implement ICollection, but implement Add method is in change request #33. Hope this helps.

[zachsaw]

Sorry for the late reply. I've been really busy the last week and haven't had time to look at the PRs. I'll take a look as soon as I have some spare time. The serialiser is currently used in a lot of IEnumerable properties / fields that won't be able to take a performance hit, but since it's pure IEnumerable, it can take ICollection just fine.

I haven't checked the PR but would the proposed changes slow down existing code?

[zachsaw]

Going back to the support of Collection(IEnumerable items), I think we can use existing code to create a collection first, and then have another object activator to call the c'tor with the IEnumerable param, after adding the items to the collection. I think a single check should suffice?

[nikolaygekht]

I haven't checked the PR but would the proposed changes slow down existing code?

Suggested changes do not slow code for the collections that implement ICollection.

In my final implementation, I left this part intact and added another specific case for IEnumerable without ICollection implementation.

In short, if before the implementation was:

if (obj is ICollection<T> xxx) 
   ... initialize collection using ICollection.Add
else 
   ... return empty collection

After change the implementation is

if (obj is ICollection<T> xxx) 
   ... initialize collection using ICollection.Add
else if (obj is IEnumerable<T> xxx)
   ... initialize collection using reflection
else
   ... return empty collection

So, ICollections work as fast as before. Anyway, I also validated the performance running benchmarking and comparing it with the previous version.

The branch for IEnumerables, which uses reflection, is 4-6 times slower, but it affects only the classes which do not implement ICollection, i.e. classes that couldn't be deserialized at all before, so we have nothing to compare :-).

I think we can use existing code to create a collection first, and then have another object activator to call the c'tor with the IEnumerable param, after adding the items to the collection. I think a single check should suffice?
Ok, let me explain the problem I was trying to avoid by my first implementation in a little bit more details.

When the object is serialized, the serializer checks only IEnumerable interface and writes the code HEnumerable, no matter does the collection support ICollection or not.

When the object is deserealized, the reader checks only HEnumerable code and then does not do any check expecting that the object implements ICollection. The first check happens all the way down the call chain, in GenericReader.ReadHEnumerable method.

The problem is that when we know that the object is not ICollection, we are all the way in the large switch by the type of the collection element. In other words, there are dozens locations where the code checks whether ICollection is implemented.

So, the problem was not to implement the collection deserialization but to avoid writing other dozens of branches, now for IEnumerable/reflection. Unfortunately, as my first attempt demonstrated, it had a cost of 2x slow down.

There were two options I hoped would help me to avoid this massive replication of the code:

Option 1) Implement a new serialization code for IEnumerable that do not support ICollection.

Option 2) Implement the type check in Deserializer.ReadValue for HEnumerable (but it will be ugly as it will create very specific way to process this particular typecode comparing to other types). I am strongly against such irregularities in the implementation.

However, after reviewing both options, I realized that I would have to replicate the collection creation code for each type of collection element, so no real code simplification would happen in fact.

[zachsaw]

I'd justify performance over DRY at this level. If there's template metaprogramming like C++, we could avoid a lot of this code duplicate.

I think I understand the problem - that is, we create a collection regardless of whether the concrete class only implements IEnumerable and not ICollection and discard the collection eventually because we can't assign that to the concrete class which only implements IEnumerable (let's call this CustomList), correct?

What I proposed was that we don't change the existing code which creates the List (collection), but we make the code create a CustomList (via object activator, of course) and pass in our List as the ctor's single param at the point where we need to assign it to the target object. Obviously, we'd need to cache CustomList object activator somewhere (perhaps the same ObjectActivator dictionary, making it return both an ImmediateObjectActivator func and a DeferredObjectActivator func - where in existing cases we have DeferredObjectActivator = null).

Hope that makes sense?

[nikolaygekht]

Hm...

This makes sense for me if we speak the other issue (#32), for the read-only structures, where deferred activator would read the state into expando object and then find the best matching constructor as Json deserializer do. So, I can recognize the activator dictionary for ordinary objects, but not for the list.

Looking at the call chain of collection deserialization, I can't identify the place where DefferedObjectActivator may come into play. Right now the collection of the concrete type is created by the call of GenericReader.CreateResultObject<T, TElement>() which calls EnumerableResultObjectCreator<T, TElement>.Creator.Create(). E.g. as in GenericReader.cs:837. This value will be filled and returned without any further assignment or conversions. So, simply I don't see the place where a DefferedObjectActivator may be actually called.

[nikolaygekht]

Merge request #37 is opened instead of the old one.

1. New implementation for IEnumerable deserialization (as discussed in old request)
2. Fixes for nullable types (bugs accidently found during debugging the new implementation :-))

[nikolaygekht]

I confirm that it is fixed in current master