Initial work on Traversal Improvements in GraphqlSchema builds #2463

bbakerman · 2021-07-15T08:17:23Z

This is a WORK implementation of building a GraQhlSchema object with only less traversal

At this stage I left the OLD code in there so you could see what's different

It would be cleaned up if we accepted this code.

I copied the bench mark from the previous PR and run it.

It shows that this code as is BETTER - but not ground breakingly better.

Start

Benchmark                                    Mode  Cnt   Score   Error  Units
SchemaBenchMark.benchMarkLargeSchemaCreate  thrpt   15  18.134 ± 1.099  ops/s

This work as is

Benchmark                                    Mode  Cnt   Score   Error  Units
SchemaBenchMark.benchMarkLargeSchemaCreate  thrpt   15  19.771 ± 0.685  ops/s

I then took out the validation steps just to see what would happen to the numbers.

Benchmark                                    Mode  Cnt   Score   Error  Units
SchemaBenchMark.benchMarkLargeSchemaCreate  thrpt   15  25.906 ± 0.370  ops/s

I then took out the type replacement step and validation just to see. This is not valid of course

Benchmark                                    Mode  Cnt   Score   Error  Units
SchemaBenchMark.benchMarkLargeSchemaCreate  thrpt   15  30.256 ± 1.171  ops/s

Having the ability to skip validation would help. Type replacements MUST happen.

If we could find a way to combine type replacement with the previous steps then it would be better. But that might
be impossible - we need ALL the types collected to do type ref replacements... OR do we. ??? Anyway that's for another PR based on this one

bbakerman · 2021-07-15T08:18:27Z

src/main/java/graphql/schema/CodeRegistryVisitor.java

 import static graphql.util.TraversalControl.CONTINUE;

 /**
 * This ensure that all fields have data fetchers and that unions and interfaces have type resolvers


I moved this to a schem.impl package just to clean things up. Its kinda of a private thing and not general purpose

bbakerman · 2021-07-15T08:18:48Z

src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java

@@ -1,12 +1,26 @@
-package graphql.schema;


Again I moved this to a schem.impl package just to clean things up. Its kinda of a private thing and not general purpose

bbakerman · 2021-07-15T08:19:26Z

src/main/java/graphql/schema/impl/MultiReadOnlyGraphQLTypeVisitor.java

+
+import java.util.List;
+
+public class MultiReadOnlyGraphQLTypeVisitor implements GraphQLTypeVisitor {


Allows us to call N visitors at once - read only ones that can change nodes

Is that public api?

I am not so sure about the semantics here: they don't compose completely, right? I can't return a ABORT/Quit anymore, for example.

They dont compose generically - they run one after the other. I have made it @Internal and improved the javadoc on what it does

bbakerman · 2021-07-15T08:19:42Z

src/main/java/graphql/schema/impl/SchemaUtil.java

@@ -1,11 +1,24 @@
-package graphql.schema;
+package graphql.schema.impl;


Again again I moved this to a schem.impl package just to clean things up. Its kinda of a private thing and not general purpose

bbakerman · 2021-07-15T08:20:01Z

src/main/java/graphql/schema/impl/StronglyConnectedComponentsTopologicallySorted.java

@@ -1,7 +1,9 @@
-package graphql.schema;


Again again again I moved this to a schem.impl package just to clean things up. Its kinda of a private thing and not general purpose

…uring_schema_build # Conflicts: # src/test/java/benchmark/SchemaBenchMark.java

…uring_schema_build

andimarek · 2021-07-18T04:40:35Z

src/main/java/graphql/schema/GraphQLSchema.java

+
+            final GraphQLSchema finalSchema = new GraphQLSchema(partiallyBuiltSchema, codeRegistry, allTypes, interfaceNameToObjectTypes);
+            schemaUtil.replaceTypeReferences(finalSchema);
+            if (true) {


this doesn't look right

Yeah as discussed direct - this was to show the new code versus the old.

The PR now has this removed and cleaned up

andimarek · 2021-07-18T04:41:01Z

src/main/java/graphql/schema/GraphQLSchema.java

+     */
+    @Internal
+    public GraphQLSchema(GraphQLSchema partiallyBuiltSchema,
+                         GraphQLCodeRegistry codeRegistry, ImmutableMap<String, GraphQLNamedType> typeMap,


it should be one line per arg, right?

andimarek · 2021-07-18T04:42:34Z

src/main/java/graphql/schema/impl/MultiReadOnlyGraphQLTypeVisitor.java

+
+import java.util.List;
+
+public class MultiReadOnlyGraphQLTypeVisitor implements GraphQLTypeVisitor {


Is that public api?

andimarek · 2021-07-18T04:46:37Z

src/main/java/graphql/schema/impl/MultiReadOnlyGraphQLTypeVisitor.java

+
+import java.util.List;
+
+public class MultiReadOnlyGraphQLTypeVisitor implements GraphQLTypeVisitor {


I am not so sure about the semantics here: they don't compose completely, right? I can't return a ABORT/Quit anymore, for example.

…uring_schema_build # Conflicts: # src/main/java/graphql/schema/SchemaTransformer.java

andimarek · 2021-07-19T19:52:41Z

src/main/java/graphql/schema/impl/SchemaUtil.java

+    }
+
+    public static void replaceTypeReferences(GraphQLSchema schema) {
+        final Map<String, GraphQLNamedType> typeMap = schema.getTypeMap();


I am not sure about this code: it looks suspicious that we grab all types to iterate. I think we can do better and maybe this is not even completely correct. But we can leave it for another PR.

Initial work on Traversal Improvements in GraphqlSchema builds

70db347

bbakerman added this to the 17.0 milestone Jul 15, 2021

bbakerman commented Jul 15, 2021

View reviewed changes

bbakerman added the performance work that is primarily targeted as performance improvements label Jul 15, 2021

Merge remote-tracking branch 'origin/master' into reduce_traversing_d…

38fb785

…uring_schema_build # Conflicts: # src/test/java/benchmark/SchemaBenchMark.java

andimarek approved these changes Jul 16, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into reduce_traversing_d…

4347d46

…uring_schema_build

andimarek self-requested a review July 18, 2021 04:40

andimarek requested changes Jul 18, 2021

View reviewed changes

bbakerman added 2 commits July 18, 2021 17:09

Merge remote-tracking branch 'origin/master' into reduce_traversing_d…

8ee36d9

…uring_schema_build # Conflicts: # src/main/java/graphql/schema/SchemaTransformer.java

More work on refactoring o schema to be faster

1ebd35f

andimarek approved these changes Jul 19, 2021

View reviewed changes

bbakerman merged commit 128e3d5 into master Jul 19, 2021


		import java.util.List;

		public class MultiReadOnlyGraphQLTypeVisitor implements GraphQLTypeVisitor {

		@@ -1,11 +1,24 @@
		package graphql.schema;
		package graphql.schema.impl;

Initial work on Traversal Improvements in GraphqlSchema builds #2463

Initial work on Traversal Improvements in GraphqlSchema builds #2463

Uh oh!

Conversation

bbakerman commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbakerman commented Jul 15, 2021 •

edited

Loading