We’re working with a model that has around 6000 objects in the Revit Structural Framing category, and we are finding the conversion to be quite slow (7-8mins).
It seems like there would be some room for improvement on this check for the presence of hosting objects in the ContextObjects. In our case, sending the Structural Framing category, the host (Level) is not included in the ContextObjects by default, but we’re still checking a list at O(n) time for every object being converted.
This index search stems from the ContextObjects being a List<ApplicationObject>. Would there be value in storing them in a different data structure? E.g. Dictionary<string, List<ApplicationObject>> (where string could perhaps be the Descriptor property) where we could get an O(1) check if an object type is present before searching through the list to find if the particular object exists. This seems like it would help performance when only selected categories are being sent.
This would of course require changing of the ISpeckleConverter, which may have impacts on other connectors that I’m not aware of. ISpeckleConverter.SetContextObjects would need to handle some sorting of objects initially with O(n) time, but it feels this would scale better for really large models?
Thought I’d open this thread for discussion as I’m sure I’m missing some considerations here!
Yeah that is surprisingly slow. We’ve been able to send that many objects or more with much much better performance. For structural framing, I would think that typically the host should be null and return true from the method that you’ve pointed out. At least that was the design intent. Do you know for certain that the host object isn’t null for your structural framing? If so we need to fix that because the host elements are meant to be physical elements such as a walls or floors in this context.
Regardless of whether the conversion is looping through this list every time or not, something is begging to be fixed here. Would you be able to share this model with me so I can take a look at it?
I would need to check if I’m able to share the model, as I’m reviewing on behalf of someone. Here is a screenshot of the parameter coming into the FamilyInstanceToSpeckle method:
You can see that there is a host object associated with the family instance obj.
Are the larger models you’ve tested with available anywhere for distribution? It would be good to do a comparison against the types of typical models that are being sent within our firm, and it could perhaps highlight some areas where we are struggling comparatively.
Ah okay yeah you are right! That’s a really good find. You’ve actually discovered a bug that I put in there so thank you
Probably the best thing to do in this specific scenario is to have that method return true if the host element is a level. That is probably going to cover the vast majority of cases for your 6000 object model plus it doesn’t involve any searching.
Now to answer your original question about searching the list, it’s actually a bit of a philosophical question that you’ve brought up. I think you have definitely discovered one of the many areas where we can do some performance optimization. I’m not sure where this quote originally came from but developers often say that “premature optimization is the root of all evil”. Basically what that means is that people tend to spend too much time optimizing methods before they even know if those methods are causing any performance issues. This change that you’re suggesting will almost certainly give some performance benefits, but if I’m unable to reproduce the performance issue and verify that this method is an issue, then I’m hesitant to make this change because of some of the things you mentioned like changes to ISpeckleConverter that would affect every converter.
And no, I don’t think that we do have any sort of public storage for Revit models although that is an interesting idea.
Probably the best thing to do in this specific scenario is to have that method return true if the host element is a level. That is probably going to cover the vast majority of cases for your 6000 object model plus it doesn’t involve any searching.
Yep, this would certainly solve all the cases for the Structural Framing category in this model. I don’t have a ton of knowledge about Revit’s hosting system, but I assumed this might crop with other categories that would require a more generic solution. If this isn’t the case the simple check would work!
Basically what that means is that people tend to spend too much time optimizing methods before they even know if those methods are causing any performance issues. This change that you’re suggesting will almost certainly give some performance benefits, but if I’m unable to reproduce the performance issue and verify that this method is an issue, then I’m hesitant to make this change because of some of the things you mentioned like changes to ISpeckleConverter that would affect every converter.
I did some quick debugging and was finding the linear list search was taking about 20ms per object. So for 6000+ objects it can definitely add up. That being said, if this is the only Revit Category that has this issue, the boolean check you mentioned above will suffice!
