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

Improve IncludeXmlComments performance #2443

Closed

Conversation

stevendarby
Copy link

@stevendarby stevendarby commented Jun 19, 2022

This PR seeks to address the performance issues highlighted in #2164

The slow performance mainly stems from the use of SelectSingleNode to search for member nodes. By creating a dictionary of all the member nodes once, the nodes can later be looked up against this dictionary much more efficiently.

In one app I work on - with a few hundred endpoints and lots of XML comments - this took swagger generation down from 25 seconds to 4 seconds.

I've tried to keep changes to a minimum and backwards compatible to increase chances of an early patch or minor release. For example, the existing set of constructors on the filters remain for external backwards compatibility but are no longer used within swashbuckle itself. The new set of constructors take a dictionary instead, which allows one to be created in the extension method and shared between all of the filters.

You may also get some further gains by fully parsing the XML document into a model of some kind, but I think that would be unnecessary as it was really the search for member nodes that was the killer.

@stevendarby
Copy link
Author

@martincostello
Copy link
Collaborator

If you'd like to reopen this PR (and rebase it), we'll take a look into it for you.

@stevendarby
Copy link
Author

Hi @martincostello, I partly closed this because I had some concerns about memory consumption and lost confidence in it, and didn't have the time (or support) to properly investigate and measure it. There's a chance I may be able to rebase and tidy up but would need a good test :)

@martincostello
Copy link
Collaborator

Yep - we'd definitely need tests if the changes aren't covered by existing ones 😄

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