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

Implement "Field Selection Merging" validation #1084

Merged
merged 32 commits into from
Oct 15, 2021
Merged

Implement "Field Selection Merging" validation #1084

merged 32 commits into from
Oct 15, 2021

Conversation

frekw
Copy link
Collaborator

@frekw frekw commented Oct 12, 2021

Still WIP, haven't added caching yet.

  • caching
  • fix imports

@frekw frekw marked this pull request as ready for review October 12, 2021 21:50
@frekw
Copy link
Collaborator Author

frekw commented Oct 12, 2021

@ghostdogpr can't quite figure out how to run the benchmarks, benchmarks/run just gives me:

sbt:caliban> benchmarks/run
[error] java.lang.RuntimeException: No main class detected.
[error]         at scala.sys.package$.error(package.scala:30)
[error] stack trace is suppressed; run last benchmarks / Compile / bgRun for the full output
[error] (benchmarks / Compile / bgRun) No main class detected.
[error] Total time: 0 s, completed 12 Oct 2021, 23:59:45

so I assume I'm missing some sbt command, but if I understand https://github.com/sbt/sbt-jmh correctly I should just call run 🤷

@ghostdogpr
Copy link
Owner

jmh:run 😄 sbt tells you about it in the welcome message when you open caliban 😜

@frekw
Copy link
Collaborator Author

frekw commented Oct 12, 2021

Haha my bad. I just gaze at the pretty ASCII art :D

@frekw
Copy link
Collaborator Author

frekw commented Oct 13, 2021

Ran some very quick benchmarks. Not 100% sure of how to read this, but if I read it correctly we get a 2x slowdown vs master (which I guess is expected, since it's probably the most complex validation rule).

However, if I'm reading it right, adding memoization seems to derease performance (perhaps from worse JITing since we dynamically create functions, or overhead from running everything as effects). However, if we want to say true to XING we probably should keep it.

memoized

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   86,272 ±(99.9%) 21,610 ops/s [Average]
[info]   (min, avg, max) = (76,813, 86,272, 90,525), stdev = 5,612
[info]   CI (99.9%): [64,662, 107,882] (assumes normal distribution)

without memoization (8a224d7)

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   908,930 ±(99.9%) 38,189 ops/s [Average]
[info]   (min, avg, max) = (898,060, 908,930, 922,191), stdev = 9,917
[info]   CI (99.9%): [870,742, 947,119] (assumes normal distribution)

master

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   1759,892 ±(99.9%) 38,579 ops/s [Average]
[info]   (min, avg, max) = (1744,626, 1759,892, 1770,722), stdev = 10,019
[info]   CI (99.9%): [1721,313, 1798,471] (assumes normal distribution)

@ghostdogpr
Copy link
Owner

The degradation is quite huge (2x slower without memoization, 20x slower with 😱). For performance sensitive paths it makes sense to use mutable variables and no effects (I did it in a few places, if you search newBuilder in the project you can see it), as long as it's hidden behind the public interface.

@frekw
Copy link
Collaborator Author

frekw commented Oct 14, 2021

Ok, I just realized I ran the validation at the wrong time (per field validation instead of once for the entire document). This improves the perf quite drastically.

I setup some different benchmarks:

  • Plain Scala
  • Plain Scala + mutable cache
  • Plain ZIO
  • Plain ZIO, memoized
  • Plain ZIO, cached

Plain Scala

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   1394,090 ±(99.9%) 111,491 ops/s [Average]
[info]   (min, avg, max) = (1353,555, 1394,090, 1421,657), stdev = 28,954
[info]   CI (99.9%): [1282,599, 1505,581] (assumes normal distribution)

Plain cached

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   1317,642 ±(99.9%) 35,427 ops/s [Average]
[info]   (min, avg, max) = (1303,303, 1317,642, 1325,627), stdev = 9,200
[info]   CI (99.9%): [1282,215, 1353,070] (assumes normal distribution)

ZIO Plain

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   1175,154 ±(99.9%) 103,335 ops/s [Average]
[info]   (min, avg, max) = (1129,664, 1175,154, 1200,016), stdev = 26,836
[info]   CI (99.9%): [1071,819, 1278,489] (assumes normal distribution)

ZIO Memoized

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   843,651 ±(99.9%) 132,545 ops/s [Average]
[info]   (min, avg, max) = (801,143, 843,651, 896,556), stdev = 34,422
[info]   CI (99.9%): [711,105, 976,196] (assumes normal distribution)

ZIO Cached

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   1233,792 ±(99.9%) 55,053 ops/s [Average]
[info]   (min, avg, max) = (1208,562, 1233,792, 1241,772), stdev = 14,297
[info]   CI (99.9%): [1178,739, 1288,845] (assumes normal distribution)

MASTER

[info] Result "caliban.GraphQLBenchmarks.introspectCaliban":
[info]   1514,755 ±(99.9%) 29,599 ops/s [Average]
[info]   (min, avg, max) = (1508,280, 1514,755, 1524,106), stdev = 7,687
[info]   CI (99.9%): [1485,156, 1544,354] (assumes normal distribution)

Looking at the stdev, I think Plain Scala and Cached Scala is about the same. The latter probably performs better with deeploy complex queries.

ZIO Cached is nice since it gets us stack safety, you pointed out. It does add some additional complexity though. Most of the versions are available as individual commits. But what's nice is that all of them are miles ahead of Sangria.

@frekw
Copy link
Collaborator Author

frekw commented Oct 15, 2021

Benchmark:

Sangria:

[info] Result "caliban.GraphQLBenchmarks.fragmentsSangria":
[info]   120,681 ±(99.9%) 6,375 ops/s [Average]
[info]   (min, avg, max) = (118,264, 120,681, 122,663), stdev = 1,656
[info]   CI (99.9%): [114,306, 127,057] (assumes normal distribution)

Caliban w/o cache:

[info] Result "caliban.GraphQLBenchmarks.fragmentsCaliban":
[info]   629,240 ±(99.9%) 65,920 ops/s [Average]
[info]   (min, avg, max) = (604,819, 629,240, 650,807), stdev = 17,119
[info]   CI (99.9%): [563,320, 695,160] (assumes normal distribution)

Caliban cached:

[info] Result "caliban.GraphQLBenchmarks.fragmentsCaliban":
[info]   593,201 ±(99.9%) 79,650 ops/s [Average]
[info]   (min, avg, max) = (562,467, 593,201, 615,912), stdev = 20,685
[info]   CI (99.9%): [513,551, 672,851] (assumes normal distribution)
[info] # Run complete. Total time: 00:00:31

@frekw
Copy link
Collaborator Author

frekw commented Oct 15, 2021

Part of #7.

@frekw frekw changed the title feat: XING field merging feat: Implement "Field Selection Merging" validation Oct 15, 2021
@frekw frekw changed the title feat: Implement "Field Selection Merging" validation Implement "Field Selection Merging" validation Oct 15, 2021
@ghostdogpr ghostdogpr merged commit 481983b into ghostdogpr:master Oct 15, 2021
@frekw frekw deleted the feat/xing branch October 16, 2021 00:38
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