Skip to content
This repository was archived by the owner on May 12, 2026. It is now read-only.

Feat(Revit) : send performance enhancement#2825

Merged
connorivy merged 11 commits into
devfrom
revit/connor/send-performance-enhancement
Aug 9, 2023
Merged

Feat(Revit) : send performance enhancement#2825
connorivy merged 11 commits into
devfrom
revit/connor/send-performance-enhancement

Conversation

@connorivy

@connorivy connorivy commented Aug 2, 2023

Copy link
Copy Markdown
Contributor

Description & motivation

Changes:

  • replaced RevitAsync nuget package with custom APIContext class

    • This new version does almost the same thing as the nuget package except it does not subscribe to the revit IdlingEvent
    • This only speeds up the conversion from .5-1s. However, the idling event documentation says that subscribing to the event in a certain way (that RevitAsync is using) "can result in performance degradation for the system on which Revit is running because the CPU remains fully engaged in serving Idling events during the Revit application's downtime." So it is recommended to avoid using this event
  • Throttle Send operation

    • only refresh the UI every .15 seconds. This makes a small impact on the ui refreshing and a huge impact on performance.
  • Optimizations for "ParameterToSpeckle"

    • This method is taking the most time of any method that we're sending because it is called so many times. Therefore small optimizations to this method can have a large performance gain.
    • Broke the parameter value logic out into it's own method "GetParameterValue" to avoid calling the entire ParameterToSpeckle method just to get the value of the convertedParameter
    • Started passing the cache object into the method for more perfomant unit conversions and queries
  • Units to Speckle

    • started converting "1" to the desired units and then caching the result as the conversion factor. This is because Revit's "ConvertFromInternalUnits" method is fairly expensive.

Screenshots:

image

A previous PR lowered the conversion time for this tower from 100s to 70s. This PR lowers the conversion time even further to ~25 seconds.

Validation of changes:

All unit tests still pass

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

@teocomi teocomi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great finds!
Does caching the units conversion ratio really speed things up? Have you benchmarked it on a large model?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is borrowed from the RevitAsync library, we should credit them :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, much of it was borrowed. Does this just mean adding a comment in the class?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly!

@teocomi teocomi added enhancement New feature or request revit issues related to the revit connector. labels Aug 5, 2023
@connorivy

Copy link
Copy Markdown
Contributor Author

Does caching the units conversion ratio really speed things up? Have you benchmarked it on a large model?

Yes it does. All of my optimizations were the result of profiling a large send operation. Caching the scale factor makes a big difference because we are calling scaleToSpeckle multiple times for every object converted. In my case I was sending ~5000 objects which lets say results in calling scaleToSpeckle 10,000 times. If caching the scale factor and using that saves 1ms over using Revit's "ConvertFromInternalUnits" method like we were doing, then the compound time savings would be 10s

@teocomi

teocomi commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

Cool, then merge away after adding the needed credits on the class :)

@connorivy connorivy merged commit e516cd0 into dev Aug 9, 2023
@connorivy connorivy deleted the revit/connor/send-performance-enhancement branch August 9, 2023 19:41
@jsdbroughton

Copy link
Copy Markdown
Contributor

Already merged into dev - the conversion logic here assumes that all conversions are by a multiplication factor between 1 and desired units.

The internal Revit util ConvertFromInternalUnits for temperature for instance from internal (Kelvin) to target (e.g. Celcius) -272.15

therefore the returned value we are caching is not able to be reapplied by factor * value which for 30*C is returning -82000, say.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request revit issues related to the revit connector.

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants