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

Optimize fragment validation #2039

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Optimize fragment validation #2039

merged 3 commits into from
Dec 15, 2023

Conversation

kyri-petrou
Copy link
Collaborator

Last stretch of optimizations before 2.5.0. Recently we focused very heavily on optimizing the Executor so I thought it'd be good to take a look at validation as well.

For the most of it, validation is fairly quick with the exception when fragments / recursive fields are involved, so I spent most of the time optimizing for them.

Main optimizations:

  1. Avoiding usage of ZPure.when(condition)(effect) when effect is a method that relies on recursion (either direct or indirect).
  2. "Caching" of the hashCode for Selection and SelectionSet. We use these objects in Sets and as keys in Maps, and due to their recursive nature calculating their hash code is fairly expensive

Other minor optimizations:

  1. Change logic in Utils.cross so that we don't return the same tuple in mirror representation
  2. Short-circuit ZPure.foreachDiscard for empty collections
  3. (Executor) Avoid using .partitionMap when support for Defer is not enabled

Some of these optimizations require binary incompatible changes. Some of the methods are public, but I doubt that are used by any end-users.

series 2/x:

[info] Benchmark                             Mode  Cnt     Score    Error  Units
[info] GraphQLBenchmarks.fragmentsCaliban   thrpt    5   256.513 ±  8.843  ops/s
[info] GraphQLBenchmarks.introspectCaliban  thrpt    5  9000.317 ± 45.534  ops/s

PR:

[info] Benchmark                             Mode  Cnt      Score     Error  Units
[info] GraphQLBenchmarks.fragmentsCaliban   thrpt    5    482.216 ±   7.531  ops/s
[info] GraphQLBenchmarks.introspectCaliban  thrpt    5  12161.453 ± 139.028  ops/s

@@ -668,20 +668,13 @@ lazy val commonSettings = Def.settings(
})
)

lazy val enforceMimaCompatibility = true // Enable / disable failing CI on binary incompatibilities
lazy val enforceMimaCompatibility = false // Enable / disable failing CI on binary incompatibilities
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: I disabled this since I had to add too many filters due to the removed methods

case _ => Right(entry)
val (deferred, eager) = {
if (isDeferredEnabled) {
filteredFields.partitionMap { f =>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one's pretty good! I ran the profiler a whole ago and I remember this partitionMap being quite visible in the flamegraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah besides avoiding wrapping the steps into an Either, .map is about twice as fast as manually creating a list with a ListBuilder (which is what partitionMap uses)

@@ -69,6 +68,8 @@ object Validator {
validateRootQuery(schema)
}.runEither

private val zunit = ZPure.unit[Unit]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks at the implementation of ZPure.unit
😭

} yield ()
}
}

// Pure's implementation doesn't check if the Iterable is empty, and this is causing some performance degradation
private def validateAll[R, A, B](
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding this to ZPure so we ultimately don't need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit tricky. The default implementation of isEmpty on an Iterable is to create an Iterator and call .hasNext() on it. In cases that the collection is non-empty, this can cause a big allocation. In our case, we're only using it with Lists and Maps, which override the default .isEmpty implementation.

Having said that, I don't know of any collections that don't override the isEmpty implementation so it might not be an issue 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the lazy collections like Scala.Stream

@kyri-petrou kyri-petrou merged commit 0cd2855 into series/2.x Dec 15, 2023
10 checks passed
@kyri-petrou kyri-petrou deleted the more-optimizations branch December 15, 2023 03:52
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.

None yet

3 participants