perf(server): avoid O(n²) when building flat entity maps (#21585)
## Problem
After 2.13, server CPU stepped up and stayed up. Sentry profiling of
`POST /metadata` pins it on
`addFlatEntityToFlatEntityMapsThroughMutationOrThrow` — ~26% self-time
plus a long tail, turning metadata-write requests into multi-second
(~18s observed) operations.
The hot stack is:
```
WorkspaceMigrationValidateBuildAndRunService.computeAllRelatedFlatEntityMaps
└ getSubFlatEntityMapsByApplicationIdsOrThrow
└ addFlatEntityToFlatEntityMapsThroughMutationOrThrow
```
Every metadata migration rebuilds the twenty-standard application's flat
sub-maps — thousands of entities, across every involved metadata type —
through this util.
## Root cause
`addFlatEntityToFlatEntityMapsThroughMutationOrThrow` maintains
`universalIdentifiersByApplicationId` and deduped each insert with
`Array.includes`:
```ts
if (!existingUniversalIdentifiers.includes(flatEntity.universalIdentifier)) {
existingUniversalIdentifiers.push(flatEntity.universalIdentifier);
}
```
That scan is O(n) per insert, so building a map for an application with
`n` entities is **O(n²)**. The twenty-standard application groups
thousands of standard entities under one `applicationId`, so its sub-map
rebuild dominates.
The dedup is also redundant: the function throws `ENTITY_ALREADY_EXISTS`
at the top if the `universalIdentifier` is already in
`byUniversalIdentifier`, and every id pushed to the per-application list
is also written there. So reaching the push guarantees the id is new —
the `.includes()` is always `false`.
## Fix
Drop the scan and push directly → map building is **O(n)**. Behavior is
unchanged (the early throw already enforces uniqueness).
## Test
Adds a unit spec covering indexing, the no-`applicationId` case, the
duplicate throw, and a 20k-entity build that completes instantly (guards
against re-introducing the quadratic).
## Follow-up (separate PR)
This is the bleed-stopper. The deeper issue is that
`computeAllInvolvedApplicationIds` pulls the **entire** twenty-standard
application into the dependency set of every migration and rebuilds
those sub-maps per request instead of caching them. Worth scoping the
dependency set to referenced entities (or caching the standard-app
sub-maps), which I'll raise separately.
<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/twentyhq/twenty/pull/21585?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
This commit is contained in:
+112
@@ -0,0 +1,112 @@
|
||||
import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant';
|
||||
import { FlatEntityMapsExceptionCode } from 'src/engine/metadata-modules/flat-entity/exceptions/flat-entity-maps.exception';
|
||||
import { type SyncableFlatEntity } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-from.type';
|
||||
import { type FlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-maps.type';
|
||||
import { addFlatEntityToFlatEntityMapsThroughMutationOrThrow } from 'src/engine/workspace-manager/workspace-migration/utils/add-flat-entity-to-flat-entity-maps-through-mutation-or-throw.util';
|
||||
|
||||
const createMaps = (): FlatEntityMaps<SyncableFlatEntity> =>
|
||||
createEmptyFlatEntityMaps();
|
||||
|
||||
const makeFlatEntity = (
|
||||
id: string,
|
||||
universalIdentifier: string,
|
||||
applicationId?: string,
|
||||
): SyncableFlatEntity =>
|
||||
({
|
||||
id,
|
||||
universalIdentifier,
|
||||
applicationId,
|
||||
}) as unknown as SyncableFlatEntity;
|
||||
|
||||
describe('addFlatEntityToFlatEntityMapsThroughMutationOrThrow', () => {
|
||||
it('should index a new entity by universalIdentifier and id', () => {
|
||||
const flatEntityMapsToMutate = createMaps();
|
||||
const flatEntity = makeFlatEntity('id-1', 'uid-1', 'app-1');
|
||||
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity,
|
||||
flatEntityMapsToMutate,
|
||||
});
|
||||
|
||||
expect(flatEntityMapsToMutate.byUniversalIdentifier['uid-1']).toBe(
|
||||
flatEntity,
|
||||
);
|
||||
expect(flatEntityMapsToMutate.universalIdentifierById['id-1']).toBe(
|
||||
'uid-1',
|
||||
);
|
||||
expect(
|
||||
flatEntityMapsToMutate.universalIdentifiersByApplicationId['app-1'],
|
||||
).toEqual(['uid-1']);
|
||||
});
|
||||
|
||||
it('should append entities that share an applicationId without duplicating', () => {
|
||||
const flatEntityMapsToMutate = createMaps();
|
||||
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity: makeFlatEntity('id-1', 'uid-1', 'app-1'),
|
||||
flatEntityMapsToMutate,
|
||||
});
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity: makeFlatEntity('id-2', 'uid-2', 'app-1'),
|
||||
flatEntityMapsToMutate,
|
||||
});
|
||||
|
||||
const universalIdentifiers =
|
||||
flatEntityMapsToMutate.universalIdentifiersByApplicationId['app-1'];
|
||||
|
||||
expect(universalIdentifiers).toEqual(['uid-1', 'uid-2']);
|
||||
// Distinct entities, so each universalIdentifier appears exactly once
|
||||
expect(new Set(universalIdentifiers).size).toBe(
|
||||
universalIdentifiers?.length,
|
||||
);
|
||||
});
|
||||
|
||||
it('should not index entities that have no applicationId', () => {
|
||||
const flatEntityMapsToMutate = createMaps();
|
||||
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity: makeFlatEntity('id-1', 'uid-1'),
|
||||
flatEntityMapsToMutate,
|
||||
});
|
||||
|
||||
expect(flatEntityMapsToMutate.universalIdentifiersByApplicationId).toEqual(
|
||||
{},
|
||||
);
|
||||
});
|
||||
|
||||
it('should throw when the universalIdentifier already exists', () => {
|
||||
const flatEntityMapsToMutate = createMaps();
|
||||
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity: makeFlatEntity('id-1', 'uid-1', 'app-1'),
|
||||
flatEntityMapsToMutate,
|
||||
});
|
||||
|
||||
expect(() =>
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity: makeFlatEntity('id-1-bis', 'uid-1', 'app-1'),
|
||||
flatEntityMapsToMutate,
|
||||
}),
|
||||
).toThrow(
|
||||
expect.objectContaining({
|
||||
code: FlatEntityMapsExceptionCode.ENTITY_ALREADY_EXISTS,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should retain every identifier when many entities share an applicationId', () => {
|
||||
const flatEntityMapsToMutate = createMaps();
|
||||
const entityCount = 20_000;
|
||||
|
||||
for (let index = 0; index < entityCount; index++) {
|
||||
addFlatEntityToFlatEntityMapsThroughMutationOrThrow({
|
||||
flatEntity: makeFlatEntity(`id-${index}`, `uid-${index}`, 'app-1'),
|
||||
flatEntityMapsToMutate,
|
||||
});
|
||||
}
|
||||
|
||||
expect(
|
||||
flatEntityMapsToMutate.universalIdentifiersByApplicationId['app-1'],
|
||||
).toHaveLength(entityCount);
|
||||
});
|
||||
});
|
||||
+1
-5
@@ -46,11 +46,7 @@ export const addFlatEntityToFlatEntityMapsThroughMutationOrThrow = <
|
||||
];
|
||||
|
||||
if (isDefined(existingUniversalIdentifiers)) {
|
||||
if (
|
||||
!existingUniversalIdentifiers.includes(flatEntity.universalIdentifier)
|
||||
) {
|
||||
existingUniversalIdentifiers.push(flatEntity.universalIdentifier);
|
||||
}
|
||||
existingUniversalIdentifiers.push(flatEntity.universalIdentifier);
|
||||
} else {
|
||||
flatEntityMapsToMutate.universalIdentifiersByApplicationId[
|
||||
flatEntity.applicationId
|
||||
|
||||
Reference in New Issue
Block a user