-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: [Factories] Config caching #7696
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
Conversation
332c8b4 to
3889325
Compare
iRedds
left a comment
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.
The idea is good, but because of the classes, the implementation looks terrible. 🥴
Controversial for me is the point of caching in index.php. I think this would be better:
- enable/disable caching via CLI command or settings in .env.
- processing to place in the core.
It would also be nice to have a CLI command that will clear any cache, and not do it manually.
Not making this caching configurable and not placing the code in the core is intentional for now. If there is a lot of agreement that it should be included in the core,
|
But this does not affect the proposed caching tool. |
1d374ab to
3699c47
Compare
TimexPeachtree
left a comment
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.
Only changing setting in index.php is bit of controversial, feature is awesome 😎.
3699c47 to
afbd366
Compare
|
Rebased. |
e6dd3c1 to
dfa206f
Compare
|
I got benchmark on Ubuntu 22.04 and Apache. Off: On: |
dfa206f to
6742144
Compare
MGatner
left a comment
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.
This is a very cool feature and impressive performance gain! I would like to see a test verifying the interaction with Registrars. We also need to determine the actual and desires intersection with our Settings library, and particularly context-specific (e.g. per user) Config values. It might just be that these are incompatible but we should be able to state that explicitly one way or another.
6742144 to
4fc5c2b
Compare
|
Updated the docs. |
MGatner
left a comment
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.
Kids interrupted, I will come back to this 😅
For Config caching.
Co-authored-by: Andrey Pyzhikov <[email protected]>
No longer use config(Config\Paths::class).
Co-authored-by: MGatner <[email protected]>
f564eea to
82328c4
Compare
|
@MGatner As far as Registrars are concerned, the Registrar works when the Config class is instantiated on the first request. That instance is then cached and used forever. As far as Settings are concerned, it is another layer on top of the Config class. |
|
🥳 |
|
System with SSD: Fisrt installation (> 5 repeats) in CodeIgniter v4.4.0. DEVELOPMENT: After DEVELOPMENT (fast ~13%): PRODUCTION: After PRODUCTION (fast ~19%): But your test fasted ~37% |
|
The benchmark results depend on the environments. My first benchmark was made on macOS (MBA), and it seems mac is much slower than Ubuntu. After all, if you want to know your truth, you need to check your production server environment. |
|
I understood. I just wanted to note that optimization does not increase equally |
Description
To improve performance.
How It Works:
Precautions:
__set_state()method.Benchmark:
Caching off:
Caching on:
Checklist: