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

load table maps lazily #1902

Merged

Conversation

mringler
Copy link
Contributor

@mringler mringler commented Aug 8, 2022

fixes #1901

When adding table classes to a Propel database object, currently those classes are immediately instantiated, event though many of those classes will not be used in one cycle, leading to unnecessary overhead, particularly on databases with lots of tables.

A simple fix is to add tables as class strings, and only instantiate them when they are actually used.

The way it is implemented here, the $tables and $tablesByPhpName properties of DatabaseMap now can contain both TableMap objects or class strings, which is not ideal, but it seems to be the most seamless integration of this change. Let me know if there is a better way.

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2022

Codecov Report

Merging #1902 (26eea73) into master (4e59c81) will increase coverage by 0.02%.
The diff coverage is 96.00%.

@@             Coverage Diff              @@
##             master    #1902      +/-   ##
============================================
+ Coverage     87.61%   87.64%   +0.02%     
- Complexity     7818     7826       +8     
============================================
  Files           227      227              
  Lines         21161    21180      +19     
============================================
+ Hits          18540    18563      +23     
+ Misses         2621     2617       -4     
Flag Coverage Δ
5-max 87.64% <96.00%> (+0.02%) ⬆️
7.4 87.64% <96.00%> (+0.02%) ⬆️
agnostic 67.03% <96.00%> (+0.04%) ⬆️
mysql 68.86% <52.00%> (-0.01%) ⬇️
pgsql 68.88% <52.00%> (-0.01%) ⬇️
sqlite 66.70% <52.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Propel/Runtime/Map/DatabaseMap.php 89.70% <95.45%> (+11.27%) ⬆️
...rc/Propel/Generator/Builder/Om/TableMapBuilder.php 95.28% <100.00%> (+<0.01%) ⬆️
...time/ServiceContainer/StandardServiceContainer.php 86.98% <100.00%> (ø)
src/Propel/Common/Util/SetColumnConverter.php 100.00% <0.00%> (ø)
src/Propel/Generator/Manager/AbstractManager.php 77.20% <0.00%> (ø)
...l/Generator/Behavior/Delegate/DelegateBehavior.php 90.22% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mringler mringler force-pushed the feature/lazy_table_map_instantiation branch from a0df3bb to b64f3dd Compare August 8, 2022 22:45
@mringler mringler force-pushed the feature/lazy_table_map_instantiation branch 2 times, most recently from 508520c to 3646296 Compare August 8, 2022 23:11
@mringler mringler force-pushed the feature/lazy_table_map_instantiation branch from 3646296 to 26eea73 Compare August 8, 2022 23:13
@mringler mringler force-pushed the feature/lazy_table_map_instantiation branch from b045f1f to 09df751 Compare August 9, 2022 16:09
@profuel
Copy link

profuel commented Aug 11, 2022

Local testing with 350 tables:
initDatabaseMaps is executed almost twice faster with this fix. Execution time of the rest of code didn't change

@dereuromark dereuromark merged commit f1fe974 into propelorm:master Aug 11, 2022
@mringler mringler deleted the feature/lazy_table_map_instantiation branch September 29, 2023 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call to initDatabaseMaps initializes too many tables.
4 participants