Skip to content
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

perf: flattenFields sanitized collection/global property, remove deep copying in validateQueryPaths #9299

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

r1tsuu
Copy link
Member

@r1tsuu r1tsuu commented Nov 18, 2024

What?

Improves querying performance of the Local API, reduces copying overhead

How?

  1. Adds flattenFields property for sanitized collection/global config which contains fields in database schema structure.
    For example, It removes rows / collapsible / unnamed tabs and merges them to the parent fields, ignores UI fields, named tabs are added as type: 'tab'.
    This simplifies code in places like Drizzle transform/traverseFields. Also, now we can avoid calling flattenTopLevelFields which adds some overhead and use collection.flattenFields / field.flattenFields.

  2. Removes this deep copying for each where query constraint

    validateSearchParam({
    collectionConfig: deepCopyObject(collectionConfig),
    errors,
    fields: fields as Field[],
    globalConfig: deepCopyObject(globalConfig),
    which potentially can add overhead if you have a large collection/global config

Benchmark in relationships/int.spec.ts:

const now = Date.now()
for (let i = 0; i < 10; i++) {
  const now = Date.now()
  for (let i = 0; i < 300; i++) {
    const query = await payload.find({
      collection: 'chained',
      where: {
        'relation.relation.name': {
          equals: 'third',
        },
        and: [
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
          {
            'relation.relation.name': {
              equals: 'third',
            },
          },
        ],
      },
    })
  }

  payload.logger.info(`#${i + 1} ${Date.now() - now}`)
}
payload.logger.info(`Total ${Date.now() - now}`)

Before:

[02:11:48] INFO: #1 3682
[02:11:50] INFO: #2 2199
[02:11:54] INFO: #3 3483
[02:11:56] INFO: #4 2516
[02:11:59] INFO: #5 2467
[02:12:01] INFO: #6 1987
[02:12:03] INFO: #7 1986
[02:12:05] INFO: #8 2375
[02:12:07] INFO: #9 2040
[02:12:09] INFO: #10 1920
    [PASS] Relationships > Querying > Nested Querying > should allow querying two levels deep (24667ms)
[02:12:09] INFO: Total 24657

After:

[02:12:36] INFO: #1 2113
[02:12:38] INFO: #2 1854
[02:12:40] INFO: #3 1700
[02:12:42] INFO: #4 1797
[02:12:44] INFO: #5 2121
[02:12:46] INFO: #6 1774
[02:12:47] INFO: #7 1670
[02:12:49] INFO: #8 1610
[02:12:50] INFO: #9 1596
[02:12:52] INFO: #10 1576
    [PASS] Relationships > Querying > Nested Querying > should allow querying two levels deep (17828ms)
[02:12:52] INFO: Total 17818

…ep copying for each constraint in `validateQueryPaths`
@r1tsuu r1tsuu marked this pull request as ready for review November 18, 2024 18:25
@denolfe denolfe removed the v3 label Nov 19, 2024
Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

This looks good! Kind of a pain to change all the names, but I think it would be good to make the name change.
What do you think?

flattenFields: FlattenField[]
} & MarkRequired<TabAsField, 'name'>

export type FlattenField =
Copy link
Contributor

Choose a reason for hiding this comment

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

The name for this type is slightly off for me which hurts readability a bit. I would name it FlattenedField and change that on these other types too. FlattenedArrayField is the result of flattenFields() the function.

* Fields in the database schema structure
* Rows / collapsible / tabs w/o name `fields` merged to top, UIs are excluded
*/
flattenFields: FlattenField[]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be named flattenedFields to be more clear. flattenFields sounds like a function.

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

Successfully merging this pull request may close these issues.

3 participants