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

Latest 9.0.7 seems to introduce indirect performance issues #211

Closed
nulltoken opened this issue Jan 28, 2021 · 18 comments
Closed

Latest 9.0.7 seems to introduce indirect performance issues #211

nulltoken opened this issue Jan 28, 2021 · 18 comments
Assignees

Comments

@nulltoken
Copy link

👋

While upgrading some indirect dependencies, we've bumped into some performance regressions.

Running time getHttpOperationsFromSpec (from prism-cli) against the same large(15Mb+) openapi 3.x document, exposing 750+ endpoints, shows a great difference in execution duration, depending on the underlying json-schema-ref-parser version.

[email protected]
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 748.239ms

[email protected]
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 1:03.202 (m:ss.mmm)

I can consistently repro this on my side. Have you noticed a similar behavior on your side? Wouldn't that be the case, I could try and build up a repro case.

/cc @P0lip @XVincentX

@P0lip
Copy link
Member

P0lip commented Jan 28, 2021

Hey!
I haven't observed that. In fact, #195 proved to be helpful in most scenarios we deal with.
There was no other meaningful change that could affect it.
BTW, did you try setting resolution to @stoplight/json-schema-ref-parser?
We backported the above change with a few twists (IIRC).
Wondering what the result would be.

@nulltoken
Copy link
Author

nulltoken commented Jan 28, 2021

@P0lip Thanks for the feedback!

BTW, did you try setting resolution to @stoplight/json-schema-ref-parser?

I've currently set my package.json resolution to

"resolutions": {
    "@stoplight/prism-cli/json-schema-ref-parser": "9.0.6"
  },

Wondering what the result would be.

I'd be happy to test this. However, I'm not sure to know the proper syntax in order to tweak the resolution so that it resolves to an entirely different package though.

How should I do this?

@P0lip
Copy link
Member

P0lip commented Jan 29, 2021

@nulltoken

"resolutions": {
  "@stoplight/prism-cli/json-schema-ref-parser": "stoplightio/json-schema-ref-parser"
}

this should work, I think.
It'll load directly from GItHub

@nulltoken
Copy link
Author

@P0lip Thanks. That didn't seem to help

With
"resolutions": {
"@stoplight/prism-cli/json-schema-ref-parser": "stoplightio/json-schema-ref-parser"
}
=> Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 1:10.836 (m:ss.mmm)

With
"resolutions": {
"@stoplight/prism-cli/json-schema-ref-parser": "9.0.6"
},
=> Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 752.841ms

I'll try to work on a repro case and submit it today.

@nulltoken
Copy link
Author

@P0lip

I'll try to work on a repro case and submit it today.

Done. See https://github.com/nulltoken/json-schema-ref-parser-repro-case

Note: As the processed endpoints are very basic, the difference in time isn't as significative as shown above, but still relevant enough to showcase the issue.

First GitHub action workflow run (https://github.com/nulltoken/json-schema-ref-parser-repro-case/runs/1805714126?check_suite_focus=true)

Running json-schema-ref-parser v9.0.7
Converting to Prism format: 4.691s
Number of processed (basic) endpoints 600

Second GitHub action workflow run (https://github.com/nulltoken/json-schema-ref-parser-repro-case/runs/1805736569?check_suite_focus=true)

Running json-schema-ref-parser v9.0.6
Converting to Prism format: 368.197ms
Number of processed (basic) endpoints 600

@P0lip
Copy link
Member

P0lip commented Feb 3, 2021

Thanks a lot!
I'll take a look to see what could cause this performance cliff.

@P0lip P0lip self-assigned this Feb 3, 2021
@P0lip
Copy link
Member

P0lip commented Feb 3, 2021

image
image
Yeah, the diff is indeed massive.

@nulltoken, I put together a candidate fix for the issue.
stoplightio@55dee7e
I tested it against a few documents, and the regression seen in case of your document is gone, while the benefits of previous solutions are still preserved (perf on par with 9.0.7 for unaffected documents).

Could you test the commit above?

@nulltoken
Copy link
Author

@nulltoken, I put together a candidate fix for the issue.

@P0lip Wow! Thanks a lot for that.

I put together a candidate fix for the issue. stoplightio/json-schema-ref-parser@55dee7e

Would you know if the yarn resolution mechanism would allow me to directly point at that commit?

@P0lip
Copy link
Member

P0lip commented Feb 3, 2021

Would you know if the yarn resolution mechanism would allow me to directly point at that commit?

  "resolutions": {
    "@stoplight/prism-cli/json-schema-ref-parser": "stoplightio/json-schema-ref-parser#slower-dereference"
  },

does the following work?

LMK. If so, what's the result on your end?

@nulltoken
Copy link
Author

does the following work?

@P0lip This fix works awesomely!

Below what I'm seeing now when dealing with the initial use case:

Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 781.798ms
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 820.296ms
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 850.51ms
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 741.779ms
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 754.58ms
Converting routing definition '2b45bad3192bfc652c94d60c869dab50f4d7a3bc' to Prism format: 830.158ms

@P0lip
Copy link
Member

P0lip commented Feb 4, 2021

Nice, glad to hear that!
I suggest to stick to that resolution then until the upstream package.
You can also stay with 9.0.6 if that's okay.

@nulltoken
Copy link
Author

I suggest to stick to that resolution then until the upstream package.

Hey! Would you have any ETA for a new release of the upstream package?

@P0lip
Copy link
Member

P0lip commented Apr 22, 2021

Hey, I have no ETA at the moment, but it's been a while since the last release, so you can expect a new one soon!
I'll let you know once it's out.

@nulltoken
Copy link
Author

[...] you can expect a new one soon!

@P0lip And how soon would be "soon"? ;-)

@nulltoken
Copy link
Author

@P0lip Kind bump?

@philsturgeon
Copy link
Member

GitHub Actions have been broken for quite some time, and despite me thinking I'd fixed them, they're back to being broken. This stops release from working. https://github.com/APIDevTools/json-schema-ref-parser/actions

If you can spot the cause let me know, I have had a few gos but am currently stumped (and off sick).

@philsturgeon
Copy link
Member

Got it! @nulltoken let me know if 9.0.8 is working a little quicker.

@nulltoken
Copy link
Author

@philsturgeon You're my hero 😍

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

No branches or pull requests

3 participants