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

Fix AJV schema initialization time #8740

Closed
medikoo opened this issue Jan 11, 2021 · 1 comment
Closed

Fix AJV schema initialization time #8740

medikoo opened this issue Jan 11, 2021 · 1 comment

Comments

@medikoo
Copy link
Contributor

medikoo commented Jan 11, 2021

Use case description

After upgrading ajv from v6 to v7 (with #8703) we've observed a slower initialization of serverless command.
It appears that schema compilation (ajv.compile(schema) execution time for very same schema jumped from ca 0.15s to 0.8s.

It also affects our test runs which locally for me jumped from 13s to 50s

I believe we need to find a way to bring back previous performance or if that will appear as not easily doable revert back to v6

Proposed solution

After discussing it in a AJV tracker -> ajv-validator/ajv#1386, it appears that solution could be to prepare a pre-compiled AJV instances for reuse (they're called "standalone" on AJV side, but they're not really standalone).

As our schema is not fixed (may be extended by plugins) we cannot simply ship serverless with pre-generated AJV validate function, still assuming that generating pre-compiled instance takes reasonable time (I didn't test it) we may take the approach in which we store it in users cache (~/.serverless/artifact) and reuse pre-compiled validate (per schema and AJV version) in all further serverless run.

Implementation proposal
  1. Seclude AJV validate resolution logic (so
    const ajv = new Ajv({ allErrors: true, coerceTypes: 'array', verbose: true, strict: false });
    require('ajv-keywords')(ajv, 'regexp');
    require('ajv-formats').default(ajv);
    and
    const validate = ajv.compile(this.schema);
    ) into lib/classes/ConfigSchemaHandler/resolve-ajv-validate.js module
  2. In lib/classes/ConfigSchemaHandler/resolve-ajv-validate.js introduce an optimized validate function resolver:
    1. Calculate hash for received schema option (i think schema may host circular references, so we should probably add some prevention for that)
    2. Relying on lib/util/getEnsureArtifact.js (for reference it's used e.g. here:
      const ensureCustomResourcesArtifact = getEnsureArtifact(artifactName, (cachePath) =>
      ) either reuse already generated validate method, or generate new one (following this instructions)

Additional notes:

  • As produced validate function source code is not really standalone (and may need access to ajv dependency) I believe we need to load it via require-from-string with a made up filename which is located in context of serverless package - that way generated validate will have access to same dependencies as serverless
  • We may consider (with other PR) refactor getEnsureArtifact so it directly resolves with memoized function, and not that it resolves with function which when invoked resolves with memoized function (I think this approach is not really needed as it was assumed and it just additionally confuses)
  • We may consider to further optimize for this case, and use different variant of getEnsureArtifact, not one that uses different folder per serveless version, but per ajv version (thanks to that, after upgrading serverless, users will not have validate rebuilt if underlying ajv was not upgraded) - and for that we may use some getEnsureArtifact generator that indeed returns function which returns a memoized function.
@medikoo
Copy link
Contributor Author

medikoo commented Jan 18, 2021

Closing, as we reverted AJV upgrade with #8762

We will revisit upgrade to AJV v7, once we have regression free path, let's handle this in context original issue #8695

@medikoo medikoo closed this as completed Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants