-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: [Factories] Config caching #7696
Conversation
332c8b4
to
3889325
Compare
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
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
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. |
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.
No longer use config(Config\Paths::class).
Co-authored-by: MGatner <mgatner@icloud.com>
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: