possible performance impact when updating from v4.2.0 to v5.0.0

[adrai]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the issue has not already been raised

Issue

Hi!

Today I was doing some dependency updates for my fastify apps, when I noticed a bigger InitDuration for my AWS Lambdas.
To have an idea my await app.ready() call went from approx. 400ms to approx. 1600ms (on AWS Lambda this went up to 18000ms and also caused some timeouts) by updating from fastify v4.1.0 to v4.2.0.

After some testing I'm assuming that probably fast-json-stringify is causing this because of this commit.

I'm not so familiar with the internals, but this simple test, helped me to compare a bit more.

For example by wrapping the refFinder(schema.$ref, location) call here: https://github.com/fastify/fast-json-stringify/blob/v4.2.0/index.js#L108 in v4.2.0
i.e. like this:

const d = Date.now()
location = refFinder(schema.$ref, location)
console.log('refFinder', Date.now() - d)

I basically get always 0 ms for the time measurements for all invocations, but compared to the newer ajvInstance.getSchema(schemaRef) call here: https://github.com/fastify/fast-json-stringify/blob/v5.0.0/index.js#L64 in v5.0.0
i.e. like this:

const d = Date.now()
ajvSchema = ajvInstance.getSchema(schemaRef)
console.log('ajvInstance.getSchema', Date.now() - d)

I'm now getting 2-20ms for the time measurements for the different invocations.

Like said, I'm not familiar with these internals, but I assume ajv.getSchema is getting the fastify.ready() call much slower.

I'm sorry for my unprofessional reproduction steps, but I hope this information may help to understand the issue.

[ivan-tymoshenko]

@adrai Hi, thank you for your report. In the commit, you mentioned we changed the way of resolving schema references because the former way to do it hasn't covered all use cases. We saw that it has some performance downgrade on cold start (reference resolving happens only in compile time), but it wasn't so dramatical #462 (comment).

If you can provide some details on a case where performance dropped from 400ms to 1600ms, it might help.

[adrai]

Just created an example repo... https://github.com/adrai/fastify-slower-schema-resolving

[mcollina]

You have a lot of schemas loaded indeed.

[adrai]

And in reality there are even more 😉

[Uzlopak]

Are there any unit tests breaking when we use refFinder instead of ajv.getSchema?

[ivan-tymoshenko]

Are there any unit tests breaking when we use refFinder instead of ajv.getSchema?

Yes, there were actually a few attempts to fix them, but it was complicated, so we ended up using Ajv for it.
Check #462

[adrai]

fyi: https://github.com/adrai/fastify-slower-schema-resolving/pull/1

[ivan-tymoshenko]

I have a suspicion that it's not refFinder that takes a lot of time, but cloning and adding schemas in Ajv.

fast-json-stringify/index.js

Line 89 in f8bdac3

schema = clone(schema)

fast-json-stringify/index.js

Line 118 in f8bdac3

ajvInstance.addSchema(externalSchema, schemaKey)

I could be wrong. Need to test.

[Eomm]

I'm working on the standalone feature because I belive this is the best approach on the long run and it was a while since I had this in mind (moreover this should be the fastest solution with tangible results).

[Uzlopak]

Doesnt ajv use internally a weakmap?

[ivan-tymoshenko]

@Eomm The other way to speed up it is to reuse context schemas. Now fast-json-schema-compiler binds context schemas and passes them to the FJS whenever it needs to build a new serializer. What we can do is pass schemas to FJS, clone and modify them only once, and then reuse them. It would be also great to figure out how to reuse one Ajv instance, but I don't have the answer now.

[Eomm]

Before any action, I would focus on:

• how many clone operations are we running for a single schema? (I bet 3 times)
• how many clone do we need?

---

If the user adds all the schema in the same context, only one serializer will be built, so this optimization is already doable by the user.

[ivan-tymoshenko]

A user can add schemas globally, and MIGHT has references on them, but it's not necessary. When we compile a route's serializer, we clone these schemas and build a new Ajv instance even if you don't have a reference on them. So if you have a lot of schemas in one namespace (added by fastify.addSchema), it might cause a performance downgrade (It is better to test it). We can think about how to reuse context schemas or maybe compile them in a lasy way.

[adrai]

I have a suspicion that it's not refFinder that takes a lot of time, but cloning and adding schemas in Ajv.

I've already tried to check this, here some numbers: https://github.com/adrai/fastify-slower-schema-resolving/tree/main/simple-benchmarks

also buildValue seems interesting 4.1.0 vs. 4.2.0

[Eomm]

We can think about how to reuse context schemas or maybe compile them in a lasy way.

Holy moly, I forgot it: we are processing externalSchemas for each route which is not necessary at all.

We could:

• use the ajv parameter as state holder. Right now we use the input ajv obj as a container, we could assume that it contains all the external schema already or add a new parameter
• update the @fastify/fast-json-stringify-compiler module to use it

[ivan-tymoshenko]

@Eomm The problem is how to add the route schema (non-external), compile a serializer, and then remove it from Ajv. Because routes schemas should be isolated from each other.

By the way, it looks like we have another problem here. Ajv compiles a validation function when we are calling getSchema.

[ivan-tymoshenko]

I found the problem. Ajv compiles a validate function at the first interaction with the schema. When we call the getSchema, it will compile a validation function even if it's not necessary. If we replace here the last line to

return { schema: sch.schema }

the performance would still be great. We can try to add an optional parameter to the Ajv.getSchema that will not force to compile the validation function, but I think it will not work.

[Uzlopak]

Maybe it is an upstream issue that @epoberezkin should know of.

[ivan-tymoshenko]

Maybe it is an upstream issue that @epoberezkin should know of.

I assume it's not an issue at all. It's by design that getSchema returns compiled schema. It's our problem that we don't need it in some cases.