-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Improve performance on metadata computation #12785
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Major refactoring to improve metadata computation performance by removing the caching lock mechanism and simplifying cache operations across the application.
- Removed
ignoreLock
parameter and associated locking logic from all cache recomputation methods, suggesting a shift towards a more efficient caching strategy without manual locks - Added
getFreshObjectMetadataMaps
as the main entry point for metadata retrieval inWorkspaceMetadataCacheService
- Introduced composite index on
workspaceId
andcreatedAt
columns inDataSourceEntity
to optimize query performance - Removed
relationMetadata
table and its constraints through migration, though migration strategy needs improvement - Enabled comprehensive TypeORM logging in core datasource, which could impact production performance
23 files reviewed, 6 comments
Edit PR Review Bot Settings | Greptile
const { objectMetadataMaps: cachedObjectMetadataMaps } = | ||
await this.workspaceMetadataCacheService.getFreshObjectMetadataMaps( | ||
{ | ||
workspaceId, | ||
}, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: this could create unnecessary recomputation overhead since getFreshObjectMetadataMaps() is not using cached values. Consider using getObjectMetadataMaps() first and only call getFreshObjectMetadataMaps() if cache is missing
packages/twenty-server/src/engine/twenty-orm/twenty-orm-global.manager.ts
Outdated
Show resolved
Hide resolved
...server/src/database/typeorm/core/migrations/common/1750673748111-remove-relation-metadata.ts
Show resolved
Hide resolved
async flush(workspaceId: string, metadataVersion?: number): Promise<void> { | ||
await this.flushVersionedMetadata(workspaceId, metadataVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider awaiting all cache deletions in parallel using Promise.all for better performance
removeUserWorkspaceRoleMap(workspaceId: string) { | ||
return this.cacheStorageService.del( | ||
`${WorkspaceCacheKeys.MetadataPermissionsUserWorkspaceRoleMap}:${workspaceId}`, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Should also remove the version when removing the map to prevent stale version references
...c/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service.ts
Show resolved
Hide resolved
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:27435 This environment will automatically shut down when the PR is closed or after 5 hours. |
await queryRunner.query( | ||
`CREATE INDEX "IDX_DATA_SOURCE_WORKSPACE_ID_CREATED_AT" ON "core"."dataSource" ("workspaceId", "createdAt") `, | ||
); | ||
await queryRunner.query( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also removing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ?
packages/twenty-server/src/engine/metadata-modules/data-source/data-source.entity.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "also introduce self recovery mecanisms to recompute cache automatically if corrupted" I don't see it? You mean you do that by comparing versions in cache VS DB? Or did you introduce something new?
Do we really want to remove ignoreLock? What about race conditions?
It seems we were already using ignoreLock: true almost everywhere so I guess it's fine- CC @ijreilly for permissions change :)
packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts
Outdated
Show resolved
Hide resolved
@@ -89,6 +94,11 @@ export class ObjectMetadataService extends TypeOrmQueryService<ObjectMetadataEnt | |||
override async createOne( | |||
objectMetadataInput: CreateObjectInput, | |||
): Promise<ObjectMetadataEntity> { | |||
const { objectMetadataMaps } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
@@ -10,6 +11,7 @@ export const buildDefaultFieldsForCustomObject = ( | |||
workspaceId: string, | |||
): Partial<FieldMetadataEntity>[] => [ | |||
{ | |||
id: v4(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because I want to avoid making multiple calls to db if not needed
...server/src/database/typeorm/core/migrations/common/1750673748111-remove-relation-metadata.ts
Show resolved
Hide resolved
I've introduced 'getFreshMetadata' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
await queryRunner.query( | ||
`CREATE INDEX "IDX_DATA_SOURCE_WORKSPACE_ID_CREATED_AT" ON "core"."dataSource" ("workspaceId", "createdAt") `, | ||
); | ||
await queryRunner.query( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ?
...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts
Show resolved
Hide resolved
...ngine/metadata-modules/workspace-metadata-cache/services/workspace-metadata-cache.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/api/graphql/workspace-schema.factory.ts
Outdated
Show resolved
Hide resolved
📊 API Changes ReportREST API ChangesSummary🔄 Changed Operations (63)
|
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
In this PR:
Improve recompute metadata cache performance. We are aiming for ~100ms
Deleting relationMetadata table and FKs pointing on it
Fetching indexMetadata and indexFieldMetadata in a separate query as typeorm is suboptimizing
Remove caching lock
As recomputing the metadata cache is lighter, we try to stop preventing multiple concurrent computations. This also simplifies interfaces
Introduce self recovery mecanisms to recompute cache automatically if corrupted
Aka getFreshObjectMetadataMaps
custom object resolver performance improvement: 1sec to 200ms
Double check queries and indexes used while creating a custom object
Remove the queries to db to use the cached objectMetadataMap
reduce objectMetadataMaps to 500kb
We used to stored 3 fieldMetadataMaps (byId, byName, byJoinColumnName). While this is great for devXP, this is not great for performances.
Using the same mecanisme as for objectMetadataMap: we only keep byIdMap and introduce two otherMaps to idByName, idByJoinColumnName to make the bridge
Add dataloader on IndexMetadata (aka indexMetadataList in the API)
Improve field resolver performances too
Deprecate ClientConfig