Synchronous Cold Start Optimization

[parisholley]

I'm making a big push to migrate my projects which include MikroORM over to serverless/edge platforms and the bootstrapping of the ORM is the only async dependency I have which causes a bit of cruft to manage global promises since async dependencies can only be initiated on the first request, then cached. I propose a modification to the internals of how entity discovery (and also metadata generation) works to make this process a little more ergonomic for non-monolithic deployments.

Bundler Friendly Metadata

As most serverless frameworks/platforms often include or abstract away the compilation process (as well as the resulting file structure) it is important that there is a blessed workflow for including all the runtime information Mikro needs via compilation vs. relying on init options that can be hard to trace down (eg: file system locations of compiled/source entities).

For my project I have a multistep process for handling metadata that eliminates that runtime headaches:

1. Use a watch command during development (or manual if user chooses) that simply executes mikro-orm cache:generate as documented to produce JSON for every discovered entity.

2. Combine all JSON entities into a single file that can be imported (and inline'd during compilation) like so:

const fs = require('fs');
const path = require('path');

// load each json file in the metadata directory and generate a new metadata object where the keys are the file names and the values are the json objects
const metadata = fs.readdirSync('./metadata').reduce((metadata, file) => {
  const name = file.replace(/\.json$/, '');
  metadata[name] = require(path.join(__dirname, 'metadata', file));
  return metadata;
}, {});

// save the metadata object to src/metadata.json so it can be imported by the client
fs.writeFileSync(path.join(__dirname, 'src/metadata.json'), JSON.stringify(metadata, null, 2));

// remove the entire metadata directory
fs.rmSync(path.join(__dirname, 'metadata'), { recursive: true });

3. Provide a GeneratedMetadataCacheAdapter that does not rely on promises and can do a simple map lookup when building the metadata storage:

import metadata from './metadata.json';

class GeneratedMetadataCacheAdapter implements CacheAdapter {
  clear(): Promise<void> {
    return Promise.resolve();
  }

  close(): Promise<void> {
    return Promise.resolve();
  }

  get(name: string): any {
    const key = `${name.replace('.js', '')}.ts`;

    if (metadata[key]) {
      return metadata[key].data);
    }

    throw new Error(`Mikro is attempt to get cache data for ${key} but it does not exist.`);
  }

  remove(name: string): Promise<void> {
    throw new Error(`Mikro is attempt to remove cache data for ${name} but Remix does not support this.`);
  }

  set(name: string): Promise<void> {
    throw new Error(`Mikro is attempt to set cache data for ${name} but Remix does not support this.`);
  }
}

"Optionally Async" Metadata Discovery

At the moment, most of the behavior that happens in MikroORM.init(), I have no need for and my life in serverless-land could be much happier if I had a way to opt-out or invoke init in a try/fail capacity. Stepping through the code path here is what I gathered:

MikroORM.init() is effectively doing 3 things that can be "opt-in" by the user just invoking what they need on the ORM instance themselves and no changes to init() are required.

• entity discover (which can be optimized to be sync based on the above)
• connecting to database
• running schema syncs/checks

That basically leaves the metadata discovery logic which can probably be tweaked a bit to support both synchronous and "opt-in" like behavior as well:

• findEntities() is pretty broad and setup in a way to handle every config/option permutation. In the case of using the generated metadata above (along with the cache provider), the only two functions in that call that are needed become discoverReferences() and discoverMissingTargets() (in my case, I pass an array of entity classes names to Mikro options).
• discoverEntity() does not need to be async since the metadata adapter above is completely in-memory. given the adapter has a strong assumption of remote/async cache management i think a new code-path is probably more appropriate than trying to tweak the cache adapter design. Granted there is JavaScriptMetadataProvider, but that still has some downstream work to turn it into a format that Mikro needs at runtime vs having an optimized metadata object generated ahead of time.
• processDiscoveredEntities() the only reason this appears to be async is for enum processing, which if that logic is happening ahead of time and included in the compilation, then this isn't needed. (i don't actually use @enum and simply rely on string types and use typescript enforcement at compiled time: eg: @Property({type:string}) foo: Bar

Based on the above, I see potentially two ways of handling this:

1. Augment the discovery code-path and replace async/awaits with a "context" object that effectively queues up a chain of promises. For example:

class MetadataContext {
  private enumInit = false;

  attemptSyncDiscovery(){
    if(this.enumInit){
      throw new Error('Cannot execute synchronous discovery since enums must be evaluated at runtime');
    }
  }

  asyncDiscovery(){
   if(this.enumInit){
     // for each filtered, await this.initEnumValues()
   }
  }

  requireEnumInit(){
    this.enumInit = true;
  }
}

// current implementation
if (prop.enum && !prop.items && prop.type && path) {
    await this.initEnumValues(meta, prop, path);
}

// alternatively
if (prop.enum && !prop.items && prop.type && path) {
  context.requireEnumInit();
}

// as a consumer, instead of 
await this.discovery.discover(this.config.get('tsNode'));

// you can try it this way
this.discovery.attemptSyncDiscovery();

2. Provide an alternative to discoverEntities() where this.metadata = await is replaced with the pre-generated/optimized JSON configuration that was done out-of-band and simply "sets" it:

    defineEntities(optimizedJson:any) {
        this.metadata = new MetadataStorage(optimizedJson);
        this.driver.setMetadata(this.metadata);
        this.em = this.driver.createEntityManager();
        this.em.global = true;
        this.metadata.decorate(this.em);
        this.driver.setMetadata(this.metadata);
    }

[B4nan]

Thanks for the suggestions, I was thinking about having a sync way to init the ORM too, but it would be more about making the async stuff later, once some of the async methods are used. But since the time for breaking changes is just now, it's a good idea to see if we can do better.

One thing I can say right ahead - the whole initEnumValues can go, I wanted to remove that in v6 anyway. The docs no longer mention the use case it was for, and over time it only caused issues, it's better to be explicit with reflect-metadata, or use ts-morph where we can extract stuff correctly without importing anything.

[B4nan]

I am not sure how to handle the caching. I think I will just change the cache adapter interface to be sync and use sync FS method variants. In the end the use should be only during the init phase and should be skipped in production with the CLI command, so it shouldn't really matter. I will keep async API available for the result cache adapter and change only the one for metadata.

Other than that, I have the initial draft almost done, but the production caching path will need more work. Maybe there could be a --combine flag in the CLI to create a single file and you would pass that explicitly in the ORM config?

[B4nan]

PR here #4166

[parisholley]

@B4nan looks good, tracks with the tweaks I had tried locally in testing it out. i think --combine works as well

[B4nan]

And #4167 will add native support for the combined cache. This will also scratch the production deployment for ts-morph, finally a way to use it while not depending on the package in production builds.

[B4nan]

I guess I will create the database connection automatically on the first execute call too (if it's not already initialized), sounds better than always enforcing the user to do it.

[B4nan]

All right, both are now merged, let me know how it works.

[parisholley]

@B4nan trying to debug issue but it appears the auto connection isn't happening. to note, i use forking a lot:

Cannot read properties of undefined (reading 'queryBuilder')
    at QueryBuilder.getKnex (..../node_modules/@mikro-orm/knex/query/QueryBuilder.js:571:30)

[parisholley]

@B4nan it appears the places you added ensureConnection() aren't executed early enough. this.getKnexQuery() expects there to be a knex client, but it will not exist (at least on postgres) until the first connect is called.

mikro-orm/packages/knex/src/query/QueryBuilder.ts

Lines 583 to 597 in d714ccc

	async execute<U = any>(method: 'all' | 'get' | 'run' = 'all', mapResults = true): Promise<U> {
	if (!this.connectionType && method !== 'run' && [QueryType.INSERT, QueryType.UPDATE, QueryType.DELETE, QueryType.TRUNCATE].includes(this.type ?? QueryType.SELECT)) {
	this.connectionType = 'write';
	}
	const query = this.getKnexQuery().toSQL();
	const cached = await this.em?.tryCache<T, U>(this.mainAlias.entityName, this._cache, ['qb.execute', query.sql, query.bindings, method]);
	if (cached?.data) {
	return cached.data;
	}
	const type = this.connectionType || (method === 'run' ? 'write' : 'read');
	const res = await this.driver.getConnection(type).execute(query.sql, query.bindings as any[], method, this.context);
	const meta = this.mainAlias.metadata;

since the AbstractSqlDriver always takes an AbstractSqlConnection, perhaps we move the ensureConnect() call into driver and out of the execute()?

[B4nan]

Don't you have a better stack trace than just this? I think em.fork shouldn't trigger anything knex related.

Cannot read properties of undefined (reading 'queryBuilder')
    at QueryBuilder.getKnex (..../node_modules/@mikro-orm/knex/query/QueryBuilder.js:571:30)

I am able to make things fail if I use the QB as the first place after, but the stack trace is different:

Cannot read properties of undefined (reading 'ref')
TypeError: Cannot read properties of undefined (reading 'ref')
    at /Users/adamek/htdocs/mikro-orm/mikro-orm/packages/knex/src/query/QueryBuilder.ts:1031:35
    at Array.map (<anonymous>)
    at QueryBuilder.finalize (/Users/adamek/htdocs/mikro-orm/mikro-orm/packages/knex/src/query/QueryBuilder.ts:1030:10)
    at QueryBuilder.getKnexQuery (/Users/adamek/htdocs/mikro-orm/mikro-orm/packages/knex/src/query/QueryBuilder.ts:491:10)
    at QueryBuilder.execute (/Users/adamek/htdocs/mikro-orm/mikro-orm/packages/knex/src/query/QueryBuilder.ts:584:24)
    at QueryBuilder.getResultList (/Users/adamek/htdocs/mikro-orm/mikro-orm/packages/knex/src/query/QueryBuilder.ts:626:26)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at initORMPostgreSql (/Users/adamek/htdocs/mikro-orm/mikro-orm/tests/bootstrap.ts:152:15)
    at Object.<anonymous> (/Users/adamek/htdocs/mikro-orm/mikro-orm/tests/EntityManager.postgre.test.ts:60:31)

Obviously, I can do this to make it fail your way, is that what you are doing somewhere?

const knex = orm.em.fork().qb(FooBar2).getKnex();

Since the knex init is sync, we should just do it eagerly, could even happen inside the connection constructor probably.