Improve performance twenty orm #6691
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request focuses on improving the performance of the Twenty ORM and implementing caching mechanisms for workspace data sources. The changes primarily affect the WorkspaceDatasourceFactory and related services.
- Implemented caching for EntitySchemas and EntitySchemaOptions in
WorkspaceDatasourceFactoryto reduce WorkspaceDataSource creation time - Added new methods
setEntitySchemaOptionsandgetEntitySchemaOptionsinWorkspaceCacheStorageServicefor efficient schema caching - Introduced performance monitoring in
MessagingMessagesImportCronJobandMessagingMessageChannelSyncStatusMonitoringCronJobusing console.time() - Improved error handling in
FileControllerfor better client feedback on stream errors - Simplified the query for fetching message channels in
MessagingMessageListFetchCronJob, potentially affecting data transfer efficiency
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| workspaceId, | ||
| ); | ||
|
|
||
| let cachedEntitySchemas: EntitySchema[]; |
There was a problem hiding this comment.
style: Consider using const instead of let for cachedEntitySchemas
| @@ -44,6 +44,10 @@ export class FileController { | |||
| workspaceId, | |||
| ); | |||
|
|
|||
| fileStream.on('error', () => { | |||
There was a problem hiding this comment.
I have noticed that local driver (at least) does not throw FileNotFound as the stream get piped. do we have to change the API here of our file drivers or is there a way to make it work like the s3 driver?
@Weiko any idea?
There was a problem hiding this comment.
I would update the read function in local driver yes. I thought it was done though, my bad! @charlesBochet
| @@ -78,18 +78,43 @@ export class WorkspaceDatasourceFactory { | |||
| ); | |||
|
|
|||
| if (!dataSourceMetadata) { | |||
| throw new Error('Data source metadata not found'); | |||
| throw new Error( | |||
| `Data source metadata not found for workspace ${workspaceId}`, | |||
There was a problem hiding this comment.
adding a bit more logs to ease debugging
| ); | ||
| } | ||
|
|
||
| const cachedEntitySchemaOptions = |
There was a problem hiding this comment.
we are caching the entity schema options (this is the typeOrmNaming, not sure it's super clear)
| @@ -69,5 +70,7 @@ export class MessagingMessagesImportCronJob { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| console.timeEnd('MessagingMessagesImportCronJob time'); | |||
There was a problem hiding this comment.
adding some temporary timers to troubleshoot performances
| @@ -44,6 +44,10 @@ export class FileController { | |||
| workspaceId, | |||
| ); | |||
|
|
|||
| fileStream.on('error', () => { | |||
There was a problem hiding this comment.
I would update the read function in local driver yes. I thought it was done though, my bad! @charlesBochet
@Weiko I have found another issue in the way we invalidate the cache, I'll add it Regarding the file driver, I would like both drivers to have the same behavior and I don't get why the s3 is throwing exceptions while the local is not ; I think the s3 driver is checking the existence of the file before streaming it, I wonder if we should actually do the same for local driver manually |
Context
As we grow, the messaging scripts are experiencing performance issues forcing us to temporarily disable them on the cloud.
While investigating the performance, I have noticed that generating the entity schema (for twentyORM) in the repository is taking ~500ms locally on my Mac M2 so likely more on pods. Caching the entitySchema then!
I'm also clarifying naming around schemaVersion and cacheVersions ==> both are renamed workspaceMetadataVersion and migrated to the workspace table (the workspaceCacheVersion table is dropped).