Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

refactor(core): decrease number of times model definitions are built #2057

Closed
wants to merge 2 commits into from
Closed

refactor(core): decrease number of times model definitions are built #2057

wants to merge 2 commits into from

Conversation

craicoverflow
Copy link

This splits the models metadata variable into a getter/setter to prevent building all models every time the GraphbackCoreMetadata#getModelDefinitions is called.

I ran benchmarks a few times and there is an improvement in startup time:

│ Server                                        │ Requests/s │ Latency │ Throughput/Mb │ Startup time (s) │ Resident Set Size (MB) │ CPU Percentage │
│ 1.0.0-graphback-apollo-express-large-schema   │ 256.4      │ 18.97   │ 3.59          │ 5.71             │ 134.94                 │ 1.51           │
│ 0.16.2-graphback-apollo-express-large-schema  │ 252.2      │ 19.27   │ 3.53          │ 6.08             │ 135.13                 │ 1.58           │

machi1990
machi1990 reviewed Sep 16, 2020
View reviewed changes
packages/graphback-runtime-knex/tests/data/KnexDbDataProviderTest.ts Outdated
@@ -4,7 +4,8 @@
import { unlinkSync, existsSync } from 'fs';
import { buildSchema } from 'graphql';
import * as Knex from 'knex';
import { GraphbackDataProvider, GraphbackCoreMetadata, QueryFilter } from '@graphback/core';
import { GraphbackDataProvider, GraphbackPluginEngine, QueryFilter } from '@graphback/core';
import { SchemaCRUDPlugin } from '../../../graphback-codegen-schema/src';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make graphback-codegen-schema dev dependency and use that here instead of relative path?

Copy link
Author

Choose a reason for hiding this comment

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

I tried importing the actual package, but got this error, which is isolated to the test files, not sure why it is happening

Type 'SchemaCRUDPlugin' is not assignable to type 'GraphbackPlugin'.
  Types of property 'transformSchema' are incompatible.
    Type '(metadata: import("/home/ephelan/code/aerogear/graphback/packages/graphback-runtime-knex/node_modules/@graphback/codegen-schema/node_modules/@graphback/core/types/plugin/GraphbackCoreMetadata").GraphbackCoreMetadata) => import("/home/ephelan/code/aerogear/graphback/node_modules/graphql/type/schema").GraphQLSchema' is not assignable to type '(metadata: import("/home/ephelan/code/aerogear/graphback/packages/graphback-core/types/plugin/GraphbackCoreMetadata").GraphbackCoreMetadata) => import("/home/ephelan/code/aerogear/graphback/node_modules/graphql/type/schema").GraphQLSchema'.
      Types of parameters 'metadata' and 'metadata' are incompatible.
        Type 'import("/home/ephelan/code/aerogear/graphback/packages/graphback-core/types/plugin/GraphbackCoreMetadata").GraphbackCoreMetadata' is not assignable to type 'import("/home/ephelan/code/aerogear/graphback/packages/graphback-runtime-knex/node_modules/@graphback/codegen-schema/node_modules/@graphback/core/types/plugin/GraphbackCoreMetadata").GraphbackCoreMetadata'.
          Types have separate declarations of a private property 'supportedCrudMethods'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I would not expect that error. Anyway, we can ignore this comments to not derail us from getting this in.

Maybe we can open a separate issue for this?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, and when I added it as a devDependency I got a circular dependency warning, so that is possibly why:

lerna WARN ECYCLE @graphback/codegen-schema -> @graphback/core -> @graphback/codegen-schema

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that explains it.

machi1990
machi1990 reviewed Sep 16, 2020
View reviewed changes
packages/graphback-runtime-knex/tests/service/DefaultCRUDServiceTest.ts
import { migrateDB, removeNonSafeOperationsFilter } from 'graphql-migrations';
import { SQLiteKnexDBDataProvider } from '../../src/SQLiteKnexDBDataProvider';
import { SchemaCRUDPlugin } from '../../../graphback-codegen-schema';
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/aerogear/graphback/pull/2057/files#r489394668

machi1990
machi1990 reviewed Sep 16, 2020
View reviewed changes
packages/graphback-runtime-mongodb/tests/MongoDataProviderTest.ts Outdated
import { Context, createTestingContext } from "./__util__";
import { buildSchema } from 'graphql';
import { QueryFilter, GraphbackPluginEngine } from '@graphback/core';
import { SchemaCRUDPlugin } from '../../graphback-codegen-schema'
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/aerogear/graphback/pull/2057/files#r489394668

machi1990
machi1990 reviewed Sep 16, 2020
View reviewed changes
packages/graphback-runtime-mongodb/tests/__util__.ts Outdated
import { GraphbackCoreMetadata } from '@graphback/core';
import { SchemaCRUDPlugin } from "@graphback/codegen-schema";
import { GraphbackPluginEngine } from '@graphback/core';
import { SchemaCRUDPlugin } from "../../graphback-codegen-schema";
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/aerogear/graphback/pull/2057/files#r489394668

machi1990
machi1990 suggested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@machi1990 machi1990 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really good change and goes in line with a similar improvement (cache models computation during plugin execution) I've had in my mind. Thanks for adding this. I've added some minor comments.

@machi1990 machi1990 added core perf performance related issue labels Sep 16, 2020
@craicoverflow craicoverflow force-pushed the model-def-improvements branch 2 times, most recently from f9f8b22 to 98aaaaa Compare September 16, 2020 15:06
@machi1990 machi1990 mentioned this pull request Sep 17, 2020
Merged
@craicoverflow craicoverflow force-pushed the model-def-improvements branch 2 times, most recently from 741fde6 to 5916e76 Compare September 17, 2020 08:37
@wtrocki
Copy link
Contributor

wtrocki commented Sep 17, 2020 •
edited
Loading

Can we approve it but wait for the merge. If we release 1.0 in the coming days (early next week) it might be risky? Just curious if that is something that really needs to be done now considering 1.0 release etc.

@craicoverflow
Copy link
Author

That is probably the wise choice @wtrocki - this looked to be a simple change but it is having some side effects. I will get it into a "ready" state but close it so we can restore it when ready.

@craicoverflow
Copy link
Author

RelationshipBuilder blocking this from working fully:

packages/graphback-core/tests/MetadataTest.ts
  ● Model has default metadata configuration

    expect(received).toHaveLength(expected)

    Expected length: 1
    Received length: 2
    Received array:  [{"kind": "oneToMany", "owner": "Note", "ownerField": {"args": [{"astNode": {"defaultValue": undefined, "description": undefined, "directives": undefined, "kind": "InputValueDefinition", "name": {"kind": "Name", "value": "filter"}, "type": {"kind": "NamedType", "name": {"kind": "Name", "value": "CommentFilter"}}}, "defaultValue": undefined, "description": null, "name": "filter", "type": "CommentFilter"}], "astNode": {"arguments": [{"defaultValue": undefined, "description": undefined, "directives": undefined, "kind": "InputValueDefinition", "name": {"kind": "Name", "value": "filter"}, "type": {"kind": "NamedType", "name": {"kind": "Name", "value": "CommentFilter"}}}], "description": undefined, "directives": undefined, "kind": "FieldDefinition", "name": {"kind": "Name", "value": "comments"}, "type": {"kind": "ListType", "type": {"kind": "NamedType", "name": {"kind": "Name", "value": "Comment"}}}}, "deprecationReason": undefined, "description": "@oneToMany(field: 'note', key: 'noteId')
    @oneToMany(field: 'note')", "extensions": {"directives": []}, "isDeprecated": false, "name": "comments", "resolve": undefined, "subscribe": undefined, "type": "[Comment]"}, "relationFieldName": "note", "relationForeignKey": "noteId", "relationType": "Comment"}, {"kind": "oneToMany", "owner": "Note", "ownerField": {"args": [{"astNode": {"defaultValue": undefined, "description": undefined, "directives": undefined, "kind": "InputValueDefinition", "name": {"kind": "Name", "value": "filter"}, "type": {"kind": "NamedType", "name": {"kind": "Name", "value": "CommentFilter"}}}, "defaultValue": undefined, "description": null, "name": "filter", "type": "CommentFilter"}], "astNode": {"arguments": [{"defaultValue": undefined, "description": undefined, "directives": undefined, "kind": "InputValueDefinition", "name": {"kind": "Name", "value": "filter"}, "type": {"kind": "NamedType", "name": {"kind": "Name", "value": "CommentFilter"}}}], "description": undefined, "directives": undefined, "kind": "FieldDefinition", "name": {"kind": "Name", "value": "comments"}, "type": {"kind": "ListType", "type": {"kind": "NamedType", "name": {"kind": "Name", "value": "Comment"}}}}, "deprecationReason": undefined, "description": "@oneToMany(field: 'note', key: 'noteId')
    @oneToMany(field: 'note')", "extensions": {"directives": []}, "isDeprecated": false, "name": "comments", "resolve": undefined, "subscribe": undefined, "type": "[Comment]"}, "relationFieldName": "note", "relationForeignKey": "noteId", "relationType": "Comment"}]

#2023 will need to be completed before merging this. Closing for now.

@craicoverflow craicoverflow mentioned this pull request Sep 17, 2020
Open
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@machi1990 machi1990 machi1990 requested changes

@wtrocki wtrocki Awaiting requested review from wtrocki

Assignees
No one assigned
Labels
core perf performance related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@craicoverflow @wtrocki @machi1990