Skip to content

Conversation

@markusz
Copy link

@markusz markusz commented Feb 19, 2016

Plenty of people are experiencing slowness when using SwaggerUI with complex nested specs here.

I had the same problem for a project and tracked the issue down to a specific call of swagger-js: resolveAllOf. This call has several problems:

  • It is a recursive call (which is not a problem per se)
  • It is always called, regardless of actual usage of allOf tags
  • This is the biggest issue: It is called twice in each recursion (see here and here ), causing the call stack to completely explode for nested definitions

The following screenshots outline the severity of the problem

Status Quo

For my (rather simple) api-docs.json, resolveAllOf is called 104,000,000 times, resulting in a render time of 4 minutes and a CPU consumption of 90 %.

default

Deduplication and disableAllOfResolve = false

The screenshot below shows the behavior after only removing the duplicated recursion in resolveAllOf in be29806

resolveAllOf is now called "only" 6584 times, which is still too often, but does not cause problems like hanging of the whole browser. Load time is okayish with 3 seconds

This is the proposed / implemented default behavior since it does not affect functionality in any way.

deduplicatedresolveallcall

Deduplication and disableAllOfResolve = true

resolveAllOf is now not called at all. This is the best approach if allOf is not used in the swagger docs. Reduces load time to < 1s

disableallofresolving

Markus Ziller added 3 commits February 19, 2016 09:55
this.resolveAllOf has been called recursively twice which really caused
the callstack to explode, from 6,000 to > 100,000,000
Speeds up parsing significantly
@fehguy
Copy link
Contributor

fehguy commented Feb 19, 2016

Free load testing! Seems like your server did well with the 100m calls.

Indeed the allOf support can get hairy, and multiple calls do occur when there's a problem resolving something that it's looking for. I'll take a look at a couple different techniques to support the external references better and review this PR. Disabling allOf isn't an option, but handling it better and certainly avoiding making extra calls should be the goal.

@markusz
Copy link
Author

markusz commented Feb 19, 2016

The proposed solution would not change behavior unless it is explicitly disabled by setting options. disableAllOfResolve: true.

A number of APIs I worked on didn't really require the use of allOf, so an option to disable this by means of a configuration attribute like the introduced disableAllOfResolve-flag might be beneficial for a number of users.

I understand, if this does not fit your project policy, but also believe, that this is a short termed pragmatic solution without disadvantages that solves a problem which affects a number of users until a better and cleaner solution is found and implemented by someone with a deeper understanding of your codebase

@fehguy
Copy link
Contributor

fehguy commented Feb 19, 2016

Thanks--I get it. Let me see if I can fix it properly first. The allOf support should have zero impact on APIs that don't have it--if that's not the case, it's busted.

@fehguy
Copy link
Contributor

fehguy commented Feb 19, 2016

Reference #708

@fehguy
Copy link
Contributor

fehguy commented Feb 19, 2016

@markusz Do you mind testing #709 against your issue?

@markusz
Copy link
Author

markusz commented Feb 20, 2016

Will do, but probably not before tomorrow

@fehguy
Copy link
Contributor

fehguy commented Feb 22, 2016

@markusz any chance to try this out? It should solve your issue.

@markusz
Copy link
Author

markusz commented Feb 22, 2016

Not yet, but will try it out asap

@andi-itagent
Copy link

+1

@fehguy
Copy link
Contributor

fehguy commented Feb 23, 2016

@andi-itagent can you please try out #709 and report back if it addresses slowness that you may be seeing?

@markusz
Copy link
Author

markusz commented Feb 23, 2016

Hey @fehguy, sorry for the delay.

I finally managed to test #709 against my issue, but unfortunatelly it did not resolve the problem (see screenshot).

709

The problem keeps existing, since your change does not address the root of the performance issues (unnecessary recursive calls) in #708 but a different potential problem.

It seems that my proposed fixed does solve these problems according to andi-itagent

@fehguy
Copy link
Contributor

fehguy commented Feb 24, 2016

OK thanks. Can you share your definition so I can use that to test & verify things?

@markusz
Copy link
Author

markusz commented Feb 24, 2016

I will have to anonymize some stuff since it is an internal API for a customer, but I'll see to it as soon as I find time

@fehguy
Copy link
Contributor

fehguy commented Feb 25, 2016

Thank you, you can also send it via email and I will test it privately.

@markusz
Copy link
Author

markusz commented Feb 29, 2016

@fehguy sent you the docs file via email

@fehguy
Copy link
Contributor

fehguy commented Apr 21, 2016

this was fixed in 57b1563

@fehguy fehguy closed this Apr 21, 2016
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.

3 participants