Skip to content

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Nov 19, 2024

motivation:

  1. rebuilds lexicographicSortSchema() on this generic utility, better demonstrating only the sorting
  2. rebuilds extendSchemaImpl() on this utility
  3. can be used as base to more easily enhance extendSchema()/buildASTSchema()/buildSchema() to take resolvers, etc
  4. can be used to expose a generic safe mapSchemaConfig() utility
  5. improved performance -- not the initial motivation, but turns out these changes bring an 80% speed improvement to buildSchema().

@yaacovCR yaacovCR requested a review from a team as a code owner November 19, 2024 11:23
@netlify
Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 7298f67
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/67459968a7f8d6000836b710
😎 Deploy Preview https://deploy-preview-4297--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR yaacovCR force-pushed the map branch 2 times, most recently from e747264 to 23e7952 Compare November 19, 2024 19:25
@yaacovCR yaacovCR changed the title introduces mapSchema function introduces mapSchemaConfig function Nov 19, 2024
@yaacovCR
Copy link
Contributor Author

note: diff is easier to scan when hiding whitespace

@yaacovCR yaacovCR changed the title introduces mapSchemaConfig function introduces mapSchemaConfig utility function Nov 19, 2024
@yaacovCR yaacovCR force-pushed the map branch 2 times, most recently from bf0cbb1 to f6325d3 Compare November 21, 2024 21:35
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 21, 2024

This re-implementation brings a 24% performance boost to buildSchema() and a small drop in memory use.

image

I'm not sure yet what exactly brings the performance boost, I was just hoping for parity.

@yaacovCR
Copy link
Contributor Author

Just poking around a bit with deopt-explorer, which, as usual, is beyond me, but the summary for main is:

image

and the summary for map is:

image

For clarity, within the map version, we rename getWrappedType() to typeFromAST() and getNamedType() to namedTypeFromAST() because it allows us to use getNamedType() in the SchemaContext closure without a clash -- and these functions essentially are a pre-GraphQLSchema-creation version of our typeFromAST() utility with the exact same algorithm.

For clarity, I also renamed buildType() to buildNamedType().

So the thing that leaps out at me is that the number of deoptimizations in buildType()/buildNamedType() and extendSchemaImpl() drop, the former significantly. I am not 100% on why. I can only guess that splitting the functions into smaller bits helped.

@yaacovCR yaacovCR added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Nov 26, 2024
@yaacovCR
Copy link
Contributor Author

UPDATE:

  • I have separated out the commits introducing mapSchemaConfig() and those used to rebuild lexicographicSortSchema() and extendSchemaImpl(), the latter underlying buildSchema() and extendSchema().
  • The initial commit has full test coverage on its own.
  • I have introduced another "helper" method available to the mapper functions, getNamedTypes() which means that we don't have to manually track the added types and then concatenate them to the pre-existing types within extendSchemaImpl().

Benchmark results from current version are even better than before, an ~80% improvement on main:

image

@yaacovCR yaacovCR force-pushed the map branch 2 times, most recently from 746c3af to 019b6a9 Compare November 26, 2024 11:32
@graphql graphql deleted a comment from github-actions bot Nov 26, 2024
@graphql graphql deleted a comment from github-actions bot Nov 26, 2024
motivation:
1. can be used to extract common logic from extendSchemaImpl and lexicographicSortSchema
2. can be used further enhance extendSchemaImpl to take resolvers
3. can be exposed to provide a generic safe mapSchemaConfig
@yaacovCR

This comment has been minimized.

@github-actions
Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.7.canary.pr.4297.42bcd01f7c733e255ff9eab84ec8d7500dfd208a
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4297

@yaacovCR yaacovCR merged commit 57ba3a3 into graphql:main Dec 1, 2024
21 checks passed
@yaacovCR yaacovCR deleted the map branch December 1, 2024 12:49
Comment on lines +459 to +464
const stdTypeMap = new Map(
[...specifiedScalarTypes, ...introspectionTypes].map((type) => [
type.name,
type,
]),
);
Copy link

Choose a reason for hiding this comment

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

It seems this is now duplicated between

const stdTypeMap = new Map(
[...specifiedScalarTypes, ...introspectionTypes].map((type) => [
type.name,
type,
]),
);
and
const stdTypeMap = new Map(
[...specifiedScalarTypes, ...introspectionTypes].map((type) => [
type.name,
type,
]),
);

One of them should suffice :-)

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

Labels

PR: polish 💅 PR doesn't change public API or any observed behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants