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

feat: [Factories] Config caching #7696

Merged
merged 17 commits into from
Aug 21, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 12, 2023

Description
To improve performance.

How It Works:

  • Save the all Config instances in Factories into a cache file before shutdown, if the state of the Config instances in Factories changes.
  • Restore cached Config instances before CodeIgniter initialization if a cache is available.

Precautions:

  • Once cached, the cache is never expired, and not updated unless the state of the Config instances in Factories changes.
  • A change in the state of Config instances in Factories means that a new Config class is instantiated and shared in Factories.
  • Therefore, simply changing a existing Config file (or changing Environment Variables for it) will continue to use the Config instance with the old cached values. In that case, you must manually delete the cache file.
  • By default, every Config class that is cached must implement __set_state() method.

Benchmark:

Requests/sec:    328.50 (Caching off)
Requests/sec:    449.71 (Caching on)
$ php -v
PHP 8.2.8 (cli) (built: Jul  6 2023 11:16:24) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.8, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.8, Copyright (c), by Zend Technologies
$ composer update --no-dev
$ php spark env

CodeIgniter v4.3.6 Command Line Tool - Server Time: 2023-07-12 03:00:54 UTC+00:00

Your environment is currently set as production.
$ symfony server:start
$ wrk -t10 -d5s http://127.0.0.1:8000/

Caching off:

Running 5s test @ http://127.0.0.1:8000/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    31.46ms   14.84ms 159.95ms   91.07%
    Req/Sec    32.96      8.29    50.00     85.34%
  1652 requests in 5.03s, 28.38MB read
Requests/sec:    328.50
Transfer/sec:      5.64MB

Caching on:

--- a/public/index.php
+++ b/public/index.php
@@ -49,8 +49,8 @@ if (! defined('ENVIRONMENT')) {
 }
 
 // Load Config Cache
-// $factoriesCache = new \CodeIgniter\Cache\FactoriesCache();
-// $factoriesCache->load('config');
+$factoriesCache = new \CodeIgniter\Cache\FactoriesCache();
+$factoriesCache->load('config');
 // ^^^ Uncomment these lines if you want to use Config Caching.
 
 /*
@@ -79,7 +79,7 @@ $app->setContext($context);
 $app->run();
 
 // Save Config Cache
-// $factoriesCache->save('config');
+$factoriesCache->save('config');
 // ^^^ Uncomment this line if you want to use Config Caching.
 
 // Exits the application, setting the exit code for CLI-based applications
Running 5s test @ http://127.0.0.1:8000/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    23.67ms   14.49ms 171.32ms   92.59%
    Req/Sec    45.21     11.09    70.00     70.82%
  2258 requests in 5.02s, 38.80MB read
Requests/sec:    449.71
Transfer/sec:      7.73MB

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.4 labels Jul 12, 2023
@kenjis kenjis force-pushed the feat-Factories-config-cache branch from 332c8b4 to 3889325 Compare July 12, 2023 04:13
Copy link
Collaborator

@iRedds iRedds left a 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.

system/Cache/FactoriesCache.php Outdated Show resolved Hide resolved
system/Cache/FactoriesCache/FileVarExportHandler.php Outdated Show resolved Hide resolved
app/Config/Paths.php Outdated Show resolved Hide resolved
public/index.php Show resolved Hide resolved
@kenjis
Copy link
Member Author

kenjis commented Jul 14, 2023

Controversial for me is the point of caching in index.php.

Not making this caching configurable and not placing the code in the core is intentional for now.
Because this caching is not for everyone, because there are many precautions.

If there is a lot of agreement that it should be included in the core,
I will make it configurable and change it that way.

It would also be nice to have a CLI command that will clear any cache, and not do it manually.

spark cache:clear will clear the cache.

@iRedds
Copy link
Collaborator

iRedds commented Jul 14, 2023

spark cache:clear will clear the cache.

But this does not affect the proposed caching tool.

@kenjis kenjis force-pushed the feat-Factories-config-cache branch 4 times, most recently from 1d374ab to 3699c47 Compare July 22, 2023 08:13
Copy link

@TimexPeachtree TimexPeachtree left a 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 😎.

@kenjis kenjis added the stale Pull requests with conflicts label Jul 29, 2023
@kenjis kenjis force-pushed the feat-Factories-config-cache branch from 3699c47 to afbd366 Compare July 29, 2023 06:22
@kenjis kenjis removed the stale Pull requests with conflicts label Jul 29, 2023
@kenjis
Copy link
Member Author

kenjis commented Jul 29, 2023

Rebased.

@kenjis kenjis force-pushed the feat-Factories-config-cache branch from e6dd3c1 to dfa206f Compare July 29, 2023 06:50
@kenjis
Copy link
Member Author

kenjis commented Aug 8, 2023

I got benchmark on Ubuntu 22.04 and Apache.

Off:

$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     6.82ms    3.38ms  47.38ms   85.19%
    Req/Sec   152.01     38.93   333.00     60.83%
  7630 requests in 5.10s, 131.09MB read
Requests/sec:   1496.21
Transfer/sec:     25.71MB

On:

$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     5.30ms    2.87ms  38.41ms   89.55%
    Req/Sec   197.98     46.97   270.00     54.40%
  9869 requests in 5.02s, 169.57MB read
Requests/sec:   1967.81
Transfer/sec:     33.81MB
$ php -v
PHP 8.1.2-1ubuntu2.13 (cli) (built: Jun 28 2023 14:01:49) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2-1ubuntu2.13, Copyright (c), by Zend Technologies

public/index.php Outdated Show resolved Hide resolved
@kenjis kenjis force-pushed the feat-Factories-config-cache branch from dfa206f to 6742144 Compare August 9, 2023 08:49
Copy link
Member

@MGatner MGatner left a 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.

@kenjis kenjis mentioned this pull request Aug 13, 2023
22 tasks
@kenjis kenjis force-pushed the feat-Factories-config-cache branch from 6742144 to 4fc5c2b Compare August 15, 2023 01:36
@kenjis
Copy link
Member Author

kenjis commented Aug 15, 2023

Updated the docs.

Copy link
Member

@MGatner MGatner left a 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 😅

user_guide_src/source/changelogs/v4.4.0.rst Outdated Show resolved Hide resolved
user_guide_src/source/concepts/factories.rst Outdated Show resolved Hide resolved
user_guide_src/source/concepts/factories.rst Outdated Show resolved Hide resolved
@kenjis kenjis force-pushed the feat-Factories-config-cache branch from f564eea to 82328c4 Compare August 16, 2023 05:24
@kenjis
Copy link
Member Author

kenjis commented Aug 16, 2023

@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.
So I believe if there are setting data in the database, these are used. If there aren't, the cached Config object is used. So Settings work always.

@kenjis kenjis requested a review from MGatner August 17, 2023 01:29
@kenjis kenjis merged commit f4c17e8 into codeigniter4:4.4 Aug 21, 2023
52 checks passed
@kenjis kenjis deleted the feat-Factories-config-cache branch August 21, 2023 01:08
@MGatner
Copy link
Member

MGatner commented Aug 23, 2023

🥳

@neznaika0
Copy link
Contributor

System with SSD:

~$ php -v
PHP 8.2.7 (cli) (built: Jun  9 2023 06:51:32) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.7, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.7, Copyright (c), by Zend Technologies
~$ uname -a
Linux aleksandr-debian 6.4.0-3-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.4.11-1 (2023-08-17) x86_64 GNU/Linux

Fisrt installation (> 5 repeats) in CodeIgniter v4.4.0.

DEVELOPMENT:

~$ wrk -t10 -d5s http://localhost:8080/
Running 5s test @ http://localhost:8080/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    36.69ms    1.77ms  44.64ms   98.45%
    Req/Sec    27.14      4.52    30.00     71.40%
  1357 requests in 5.01s, 52.06MB read
  Socket errors: connect 0, read 1357, write 0, timeout 0
Requests/sec:    271.11
Transfer/sec:     10.40MB

After DEVELOPMENT (fast ~13%):

~$ wrk -t10 -d5s http://localhost:8080/
Running 5s test @ http://localhost:8080/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    32.44ms    1.58ms  40.37ms   96.42%
    Req/Sec    30.70      2.63    40.00     92.60%
  1535 requests in 5.01s, 58.89MB read
  Socket errors: connect 0, read 1535, write 0, timeout 0
Requests/sec:    306.60
Transfer/sec:     11.76MB

PRODUCTION:

~$ wrk -t10 -d5s http://localhost:8080/
Running 5s test @ http://localhost:8080/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    26.26ms    1.30ms  35.59ms   97.63%
    Req/Sec    37.94      4.05    40.00     79.40%
  1897 requests in 5.00s, 32.54MB read
  Socket errors: connect 0, read 1897, write 0, timeout 0
Requests/sec:    379.11
Transfer/sec:      6.50MB

After PRODUCTION (fast ~19%):

~$ wrk -t10 -d5s http://localhost:8080/
Running 5s test @ http://localhost:8080/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    21.82ms    1.10ms  33.70ms   97.37%
    Req/Sec    45.64      4.97    50.00     56.40%
  2282 requests in 5.00s, 39.14MB read
  Socket errors: connect 0, read 2282, write 0, timeout 0
Requests/sec:    455.97
Transfer/sec:      7.82MB

But your test fasted ~37%

@kenjis
Copy link
Member Author

kenjis commented Sep 6, 2023

The benchmark results depend on the environments.

My first benchmark was made on macOS (MBA), and it seems mac is much slower than Ubuntu.
See #7696 (comment)
The benchmark on Ubuntu was made on another note PC, but the spec is not much better than the MBA.

After all, if you want to know your truth, you need to check your production server environment.

@neznaika0
Copy link
Contributor

I understood. I just wanted to note that optimization does not increase equally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants