Skip to content

Conversation

@wasche
Copy link
Contributor

@wasche wasche commented May 6, 2019

before:
AddError.benchMarkAddError ss 5 1.079 ± 0.064 s/op

after:
AddError.benchMarkAddError ss 5 0.131 ± 0.059 s/op

@bbakerman
Copy link
Member

Do you have a use case where you want to create 5000 errors and you have noticed an actual slow down because of the O(n) error lookup

These types of micro benchmarks have a history of not paying off in practice.

@wasche
Copy link
Contributor Author

wasche commented May 7, 2019

The amount of throuput we do, we've run into this issue numerous times, and it effects us hitting our SLA. So yes, this has had a significant impact on performance. In an ideal world, there wouldn't be so many errors, but they are out of our control.

@samkline
Copy link

samkline commented May 7, 2019

Hi @bbakerman, thanks for looking at this. I was working with @wasche on this and at the time I had hooked up VisualVM to get some CPU profiles. The way I ran this test was to throw a bunch of queries at our GraphQL service. Each query fetched a few hundred items from one service, and then went to a few supplementary services to fill in the fields for those items. I took one of those supplementary services offline to see how the system responded. The issue we were seeing is that if one of those services went down, GraphQL's CPU usage would spike and the service would be severely degraded.

Sadly I don't have the full results anymore but I do have this screenshot (below) showing that within my test, of all the time used by ExecutionStrategy.completeField, 93% of it was spent in ExecutionContext.addError. I believe I ran the test for only a minute or two, so the 25 seconds in the screenshot represents a large amount of work.

I had added a workaround in our service when we were running 9.2, but now that we're upgrading to 12.0, our workaround no longer works (we were relying on the public constructor of ExecutionContext to subclass it and override the addError behavior).

Screen Shot 2018-12-05 at 7 22 36 AM

@bbakerman
Copy link
Member

bbakerman commented May 9, 2019 via email

@bbakerman bbakerman merged commit 9616132 into graphql-java:master May 14, 2019
@bbakerman bbakerman added this to the 13.0 milestone May 14, 2019
@bbakerman bbakerman added the needs to be backported a bugfix that still needs to be backported label May 14, 2019
bbakerman pushed a commit to bbakerman/graphql-java that referenced this pull request Jun 12, 2019
bbakerman pushed a commit that referenced this pull request Jun 12, 2019
* fixes #1524

* fixes #1524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs to be backported a bugfix that still needs to be backported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants