Skip init of Propel tables in order to boost performance

[profuel]

When database table map is initialized, originally we create all table classes. Regular request uses 5-10 tables, so creation of classes is not required.

With this change, only mapping from the qualified table name to a php class name is stored in the database, and the table map class creation happens only on-demand.

[mringler]

Looks like what you are doing is to replace the array holding the qualified names of the table classes by an associative array, which maps the common table name to the qualified name. So this:

[
  0 => '\\Propel\\Tests\\Bookstore\\Book'
]

becomes this:

[
  'book' => '\\Propel\\Tests\\Bookstore\\Book'
]

And later on, you want to use the common name from the key to avoid having to access the static property of the class:

    public function registerTableMapClass(string $tableMapClass): void
    {
        $tableName = $tableMapClass::TABLE_NAME; // <----- this line you want to avoid
        $this->tables[$tableName] = $tableMapClass;

        $tablePhpName = $tableMapClass::TABLE_PHP_NAME; // <----- btw. looks like you just removed this
        $this->addTableByPhpName($tablePhpName, $tableMapClass);
    }

From what I see, this is the only change that might impact runtime, depending on the overhead of accessing the static property.

So far, did I figure it out correctly, is that what you want to do?

If so, can you give me a rough idea of what amount of time can be saved with this?

[profuel]

Thanks for review @mringler.

Yes, you've got the idea right!

On my local running project, with 320 tables, performance win is 5-50ms. And this is a constant win for every call. So with 10 calls a second, you get 1.2-12 hours CPU time win a day.

Main goal here is to avoid any Propel database mappings, when no DB operations are expected during the call processing.

[mringler]

Ah, nice, sounds good, sorry it took me a bit to get back to you!

In your changes, you removed the possibility to get tables by php name (i.e. 'Bookstore' instead of 'bookstore_schemas.bookstore'), but I don't think we can just get rid of it. I'm pretty sure that when that works, tests will also run through.

To fix this, the loadDatabase.php file would have to also include the php names, which does not work with a simple key-value structure, but is still easy to do. Two possibilities spring to mind:

First is to just write tuples to the file, which include name, php name and class, i.e.

[
  0 => ['bookstore_schemas.book', 'Book', '\\Propel\\Tests\\Bookstore\\Book']
]

and then build the two table maps (name to table class and php name to table class) from that, which should be easy enough.

Alternatively, we could just write the two maps into the file as they are stored internally inside the DatabaseMap class, i.e. something like

$serviceContainer->initDatabaseMaps([
  'default' => [
    'tables' => [
      'bookstore_schemas.book' => '\\Propel\\Tests\\Bookstore\\Book'
    ],
    'tablesByPhpName' => [
      'Book' => '\\Propel\\Tests\\Bookstore\\Book'
    ]
  ]
])

The advantage here is that we don't have to build a second structure, but just insert the maps into the DatabaseMap, and we might be able to use existing code to generate the content of the loadDatabase.php, since we basically just write a dump to that file.

Does that make sense? Let me know what you think about it. Do you favor one of those options, or even a whole other approach?

[profuel]

No problem with review time at all, thanks Moritz!

Getting table by PHP name works anyway, if you check the function \Propel\Runtime\Map\DatabaseMap::getTableByPhpName

Though I agree, building full tables map make more sense, mentioned above function will return from the first IF much faster.

[mringler]

Hey Andrey, my plan was to ask if you can implement the changes we talked about, and I just wanted to have a quick look at how it would work. So I played around with it a bit, and it worked almost immediately. Now I fell like I grabbed it from you, but at the same time, asking you to do it again seems wrong, too. So I just created a new MR #1910. It'd be great if you have a look at it, maybe do a review, and maybe even check if does make a difference with your large database.

[profuel]

I will happily close this one, once I test your solution. thanks @mringler !

[dereuromark]

Alternative PR is open and ready to be merged.