Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inlining Element Wrapping #5

Open
wants to merge 3 commits into
base: reope/performance-base
Choose a base branch
from

Conversation

dimven-adsk
Copy link
Owner

@dimven-adsk dimven-adsk commented Aug 29, 2024

List of Affected Nodes/Modules

All nodes returning a wrapped element would benefit

Current Performance

The current implementation works perfectly fine. However the newer C# language features do provide some performance advantages here. This change does get rid of the previous public wrapper methods and might potentially cause issues if any external code was relying on them.

Proposed Performance

we manually inline all of the one-liner wrap methods into a single pattern matching switch table.

Dynamo Tuneup Comparison

On the standard arch. sample model collecting all elements of all categories five times over (about 125k element conversions in total), we can improve performance from 71 seconds on average down to about 66 seconds.

z7U8m6MpLB

d5Xc26veq6

This change might make the ElementWrapper code base less legibile.

Checklist

  • There are no public function signature changes
  • Any public code that is no longer in use is tagged as obsolete and preserved.
  • The code changes have been documented
  • The overall behavior does not change

@dimven-adsk dimven-adsk changed the title wrapping inlining Inlining Element Wrapping Sep 4, 2024

#region Wrap methods

public static UnknownElement Wrap(Autodesk.Revit.DB.Element element, bool isRevitOwned)
Copy link

Choose a reason for hiding this comment

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

We should decide if we want to deprecate all these methods and keep them one release pointing users to the ToDSType method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants