Skip to content

Jon freed/perf ziptreecont match #1583

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

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

Conversation

jon-freed
Copy link

@jon-freed jon-freed commented Jul 11, 2025

What does this PR do?

Improves performance of the ZipTreeContainer.match() function, which can be called many thousands of times during metadata operations.

The match() code being replaced does a linear search (Array.find()) while doing the same string manipulation (normalize()) for each array entry it evaluates. All of that work adds up.

The new match() code just does one string manipulation (posix.normalize) on the incoming string parameter and then does a simple and fast direct key lookup. This works because the keys are JSZip keys that are always in forward-slash (POSIX normalized) format.

The match() function inputs and outputs remain the same.

In performance tests with full-org retrievals, this change reduced match() CPU self time by 5–12%, shaving over a minute from overall execution time due to the high volume of calls.

What issues does this PR fix or reference?

n/a

Functionality Before

match() returned the path to the file within the zip file if found, otherwise undefined.

Functionality After

same

@jon-freed jon-freed requested a review from a team as a code owner July 11, 2025 01:08
@WillieRuemmele
Copy link
Member

@jon-freed - that's a noticeable performance improvement 🚀 - I'll run this through out test suite, and performance suite, and see what results we get

@WillieRuemmele
Copy link
Member

@jon-freed - I tried running just the unit tests locally and saw 15+ failures... are you seeing those as well?

  1) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should retrieve zip and extract to directory:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:246:9)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:246:9)

  2) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should retrieve zip with packages and extract to default directory:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:270:9)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at Function.invoke (node_modules/sinon/lib/sinon/proxy-invoke.js:50:47)
      at Function.fromSource (node_modules/sinon/lib/sinon/proxy.js:270:26)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:270:9)

  3) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should retrieve zip with packages and extract to specified directory:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:304:9)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at Function.invoke (node_modules/sinon/lib/sinon/proxy-invoke.js:50:47)
      at Function.fromSource (node_modules/sinon/lib/sinon/proxy.js:270:26)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:304:9)

  4) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should save the temp directory if the environment variable is set:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:337:11)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:337:11)

  5) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should NOT save the temp directory if the environment variable is NOT set:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:358:9)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:358:9)

  6) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should retrieve zip and merge with existing components:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:373:9)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:373:9)

  7) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should construct a result object with retrieved components:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:397:24)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:397:24)

  8) MetadataApiRetrieve
       Lifecycle
         pollStatus
           should construct a result object with no components when components are forceIgnored:
     MetadataTransferError: Metadata API request failed: unpackaged: File or folder not found
      at MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:415:24)
  DUE TO:
  TypeInferenceError: unpackaged: File or folder not found
      at MetadataResolver.getComponentsFromPath (src/resolve/metadataResolver.ts:22:1)
      at buildComponents (src/collections/componentSet.ts:36:1415)
      at Function.fromSource (src/collections/componentSet.ts:36:1539)
      at extract (src/client/retrieveExtract.ts:7:1911)
      at async MetadataApiRetrieve.post (src/client/metadataApiRetrieve.ts:37:41)
      at async MetadataApiRetrieve.pollStatus (src/client/metadataTransfer.ts:6:4)
      at async Context.<anonymous> (test/client/metadataApiRetrieve.test.ts:415:24)

  9) Tree Containers
       ZipTreeContainer
         exists
           should return true for directory that exists:

      AssertionError: expected false to be true
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:173:40)
      at processImmediate (node:internal/timers:485:21)

  10) Tree Containers
       ZipTreeContainer
         isDirectory
           should return true for isDirectory:
     LibraryError: main/default: File or folder not found
      at ZipTreeContainer.isDirectory (src/resolve/treeContainers.ts:25:780)
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:194:21)
      at processImmediate (node:internal/timers:485:21)

  11) Tree Containers
       ZipTreeContainer
         readDirectory
           should return correct directory entries for directory with files and directories:
     LibraryError: main/default: path is to a file, expected a directory
      at ZipTreeContainer.readDirectory (src/resolve/treeContainers.ts:27:522)
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:205:21)
      at processImmediate (node:internal/timers:485:21)

  12) Tree Containers
       ZipTreeContainer
         readDirectory
           should return correct directory entries for directory only files:
     LibraryError: main/default/morefiles: path is to a file, expected a directory
      at ZipTreeContainer.readDirectory (src/resolve/treeContainers.ts:27:522)
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:210:21)
      at processImmediate (node:internal/timers:485:21)

  13) Tree Containers
       ZipTreeContainer
         readDirectory
           should return correct directory entries for directory with only directories:
     LibraryError: main: path is to a file, expected a directory
      at ZipTreeContainer.readDirectory (src/resolve/treeContainers.ts:27:522)
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:214:21)
      at processImmediate (node:internal/timers:485:21)

  14) Tree Containers
       ZipTreeContainer
         readFile
           should throw an error if path is to directory:

      AssertionError: expected LibraryError: main/default: path is to a directory, expected a file { actions: undefined, exitCode: 1, context: undefined, data: undefined, cause: undefined } to have property 'message' of 'Expected a file at path main/default but found a directory.', but got 'main/default: path is to a directory, expected a file'
      + expected - actual

      -main/default: path is to a directory, expected a file
      +Expected a file at path main/default but found a directory.
      
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:245:31)

  15) Tree Containers
       ZipTreeContainer
         stream
           should throw an error if given path is to a directory:

      AssertionError: expected LibraryError: main/default: path is to a directory, expected a file { actions: undefined, exitCode: 1, context: undefined, data: undefined, cause: undefined } to have property 'message' of 'ZipTreeContainer doesn\'t support readable streams on directories.', but got 'main/default: path is to a directory, expected a file'
      + expected - actual

      -main/default: path is to a directory, expected a file
      +ZipTreeContainer doesn't support readable streams on directories.
      
      at Context.<anonymous> (test/resolve/treeContainers.test.ts:272:31)
      at processImmediate (node:internal/timers:485:21)

@jon-freed
Copy link
Author

jon-freed commented Jul 12, 2025

@WillieRuemmele - Try this branch after this last commit that handles trailing slashes consistently. I made the changes then did yarn build and yarn test.

I am not getting any errors that I can trace to the change in this PR. I have verified that in two ways:

First, I get the following yarn test failure(s) with this PR branch and in the current main branch. (Different code, but the same errors.)

72 passing (1m)
  2 failing

  1) a single field in a CustomObject xml does not overwrite (blank) the existing Object
       will not overwrite .object-meta.xml:

      AssertionError: expected 1 to be at least 64
      + expected - actual

      -1
      +64

      at validateSourceDir (test\snapshot\sampleProjects\singleCustomFieldRetrieve\snapshots.test.ts:263:50)
      at async Context.<anonymous> (test\snapshot\sampleProjects\singleCustomFieldRetrieve\snapshots.test.ts:77:5)

  2) a single field in a CustomObject xml does not overwrite (blank) the existing Object
       verify force-app:

      AssertionError: expected 1 to be at least 64
      + expected - actual

      -1
      +64

      at validateSourceDir (test\snapshot\sampleProjects\singleCustomFieldRetrieve\snapshots.test.ts:263:50)
      at async Context.<anonymous> (test\snapshot\sampleProjects\singleCustomFieldRetrieve\snapshots.test.ts:245:5)



❌ 1 script failed.
error Command failed with exit code 1.

Second, I did not see any thrown errors during yarn test after I temporarily changed the code in my branch as described below. This temporary change compares the match() function's return value both with and without the PR change, and it throws an error if the two return values are not exactly equal. (I got past this test without any thrown errors.)

  1. Update the import statement at the top to again include "normalize" so that the statement is this: import { join, dirname, basename, normalize, sep, posix } from 'node:path';

  2. Update the match function's code to be this:

  private match(fsPath: string): string | undefined {
    // "dot" has a special meaning as a directory name and always matches. Just return it.
    if (fsPath === '.') {
      return fsPath;
    }

    const fsPathBasename = basename(fsPath);
    const fsPathDirname = dirname(fsPath);
    const oldReturnValue = Object.keys(this.zip.files).find((filePath) => {
      const normFilePath = normalize(filePath);
      if (basename(normFilePath) === fsPathBasename) {
        return dirname(normFilePath) === fsPathDirname;
      }
    });

    const newReturnValue = this.zipKeyMap.get(posix.normalize(fsPath.replace(/\\/g, '/')));

    if (oldReturnValue !== newReturnValue)
      throw new Error(`oldReturnValue !== newReturnValue; '${oldReturnValue!}' !== '${newReturnValue!}'`);
    return oldReturnValue;
  }

@jon-freed
Copy link
Author

@WillieRuemmele - Curious if you (or anybody else) have any further questions or concerns? Thanks

@WillieRuemmele
Copy link
Member

Hey @jon-freed - we're in planning this week, I'll make sure to check it out on Monday 😄

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.

2 participants