BHoM / Speckle_Toolkit

GNU Lesser General Public License v3.0
10 stars 2 forks source link

Performance: Speckle Serialiser takes too long to complete #49

Closed alelom closed 4 years ago

alelom commented 4 years ago

Description:

Speckle_Serialiser is responsible for almost all computation time while performing the Push. We're talking several seconds per <10 Structure.Bars. I suspect it's due to a limitation of how speckle does its "serialisation" together with the deep nesting of our objects, which probably Speckle wasn't designed for. SpeckleSerialiser does in fact expose a maximumNestLevel parameter, which we cannot afford to use as we need to send eveything. (Edit: changing it does not seem to do anything).

Already highlighted the issue to Dimitri. I need to produce more detailed profiling; based on that potentially raise an issue in their Github.

In the meantime, perhaps #58 could help.

alelom commented 4 years ago

After this: https://github.com/BHoM/Speckle_Toolkit/issues/58#issuecomment-582900217

It's probably worth trying reduce the nesting level of serialisation (parameter exposed by SpeckleSerialiser). This "partial" serialisation could be used for SpeckleViewer filtering/selection of objects by property, if the nesting limit still exposes enough properties. The actual BHoM Data can be then serialised with our Serialiser, and added as an additional Property string value, so the full BHoMObject can be fully transferred.

alelom commented 4 years ago

Tried the thing explained in my previous comment. The nesting level does not seem to do anything significant with regards to speed (or anything at all).

Only thing that is left to do is expose an option that allows the user to choose whether to Push with the SpeckleSerialiser or with our serialiser. Our serialiser is ~100 faster, but we lose the ability to filter/group per BHoMObject property in the SpeckleViewer.

didimitrie commented 4 years ago

Yep, true, i can definitively reproduce; but this is always going to be an issue using the speckle abstract route because of all the home brew reflection involved.

image

the c# component just calls JsonConvert.Serlialize(x) for reference.

Speckle's serialiser is geared towards serialising Speckle objects after all; the abstract route is a convenient way of testing things without creating a full blown speckle kit.

didimitrie commented 4 years ago

I've tested making the root bhom object class inherit from the speckle object class, effectively transforming the whole of the bhom object model into a pseudo-speckle kit (for fun and giggles, as they say).

namespace BH.oM.Base
{
    // -------------------------------v   PANIC TIME
    public class BHoMObject : SpeckleObject, IBHoMObject
    {
        public Guid BHoM_Guid { get; set; } = Guid.NewGuid();
// ...

There's some objects, namely geometry constructs, that do not inherit from it - which was weird. I've made them inherit from the Bhom object base class too for giggles. This effectively allowed Core, with some minor modifications, to see bhom objects as native speckle objects :)

Serialisation time, in this case, is quite improved of course:

image

Everything's red as there's missing conventions coming from speckle that are not implemented in Bhom. Just leaving this here - imho this route is a much better integration (especially since everything else in BHoM land is working perfectly fine).

alelom commented 4 years ago

Thanks @didimitrie .

Before your latest comment, I "solved" this by re-writing the Speckle Serialise method in its "Essential" parts, so it does the bare minimum I need to get the objects "filterable by property" in the SpeckleViewer. Then the actual BHoMObject data is serialised (fast) with our serialiser, and stored in the resulting top-level abstract object (in its Property field).

This allows to get both features in (SpeckleViewer to "digest" the objects, and the full bhom object to be serialised/deserialised fast).

Tomorrow I'll review your last comment and let you know my thoughts about that. Cheers!

alelom commented 4 years ago

I've tested making the root bhom object class inherit from the speckle object class, effectively transforming the whole of the bhom object model into a pseudo-speckle kit (for fun and giggles, as they say).

I loved the PANIC TIME remark in the code 🤣

There's some objects, namely geometry constructs, that do not inherit from it - which was weird. I've made them inherit from the Bhom object base class too [...]

I completely agree that's weird. However they are reasons for that. I had advocated in the past for a different solution, but that's what we're sticking to. Your comment is precious to me as it's proving my point of back then.

imho this route is a much better integration (especially since everything else in BHoM land is working perfectly fine).

This I am not sure though. This would require literally all repos of BHoM to depend on Speckle. In principle I would have absolutely nothing against that! However, this choice would rightly need a justification – so far this would benefit only a limited number of repos, namely one: Speckle_Toolkit.