-
Notifications
You must be signed in to change notification settings - Fork 397
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
load databases without accessing classes #1910
load databases without accessing classes #1910
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1910 +/- ##
============================================
- Coverage 87.64% 87.60% -0.04%
- Complexity 7829 7839 +10
============================================
Files 227 227
Lines 21183 21193 +10
============================================
+ Hits 18565 18567 +2
- Misses 2618 2626 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/Propel/Runtime/ServiceContainer/StandardServiceContainer.php
Outdated
Show resolved
Hide resolved
de639e5
to
9a042f5
Compare
I like the idea, it looks like as a next step to optimize loading of a tables. |
I've tested this locally. So I'm fully for this solution. BTW, \Propel\Runtime\ServiceContainer\StandardServiceContainer and especially setConnectionManager has a major change, which lead to a minor change on our project side :) |
@profuel I dont see any changes to setConnectionManager() etc, can you clarify the major changes that have implications for you? |
@dereuromark my fault, we have an older Propel on the project. All good here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the analyis/QA I will now approve this
With a 2nd approve we have green light for merge.
Co-authored-by: Mark Scherer <[email protected]>
Thank you @profuel for the tests! |
@mringler my pleasure. We need this change on the project, so - mutual goal :) |
Any other approvers? I am only waiting for this for the PR to get merged. |
This MR changes the way tables are registered during initialization to improve performance. Instead of building the internal table lookup structure by accessing static class properties of the table classes, the table lookup is loaded directly from the init file.
Background
During initialization, available database tables are registered in Propel. Currently this is done through the qualified class name (i.e.
\Propel\Tests\BookstoreSchemas\Map\BookTableMap
), which is handed to aDatabaseMap
, which then adds it to its table lookup maps. There are two kinds of lookup, access by name (schema and table name, i.e.bookstore_schemas.book
) or by PHP name (either set in schema.xml or generated from name, i.e.\Book
).Currently, the lookup maps are created by accessing static properties of the supplied class. It was reported that this leads to a measurable slowdown when working with lots of tables (300+), see here.
Proposed Solution
Instead of handing over only the table class name to the DatabaseMap, we can also hand over the name and PHP name, making the static access obsolete. Even simpler, instead of reading the raw table data from the init file (usually
loadDatabase.php
) and then building up the lookup structure every time, we can just write and then retrieve that lookup structure from the init file, and simply load it into the DatabaseMap as it is.#1909 does something similar, and it reportedly leads to a speed up.
Changes
DatabaseMap
class gets a method to dump its lookup maps and a method to load them again from dump.TableMapLoaderScriptBuilder
class (which generates theloadDatabase.php
file) does not just write an array of table class names, but registers the tables in a local DatabaseMap, and then writes a dump to the init file.StandardServiceContainer
).The changes do not impact BC, even after the update, an old init file can still be used.
Result
On my machine, with PHP 8.1 and a project with 40 tables, initialization through static access takes around 0.8 ms (+- 0.3ms), while initialization through dumps takes around 0.25 ms (+- 0.1ms). I am actually surprised how clearly and distinct the difference between the two approaches is. I guess it comes from the time it takes PHP to load the classes, but there is also the repeated calls to a subroutine and writing to the lookup arrays individually. I have tested it through the php dev server, not on an actual webserver, which might give different results.
Of course we are talking about less than a thousand of a second, so this will not make a difference to most users. But it won't hurt either.
I don't have a project with more tables available, but maybe @profuel can supply some measurements for a larger set of tables? I would assume that the static approach grows linear with the number of tables, as each new table leads to another class being loaded, where the dump approach stays mostly the same, as adding another new table just means to read two more lines from a file.