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: FileLocator caching #8017

Merged
merged 18 commits into from
Oct 16, 2023
Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Oct 6, 2023

Description
To improve performance.
Benchmark: #8017 (comment)

  • add FileLocatorInterface
  • add FileLocatorCached

How to Use:

<?php

namespace Config;

use CodeIgniter\Autoloader\FileLocator;
use CodeIgniter\Autoloader\FileLocatorCached;
use CodeIgniter\Config\BaseService;

class Services extends BaseService
{
    public static function locator(bool $getShared = true)
    {
        if ($getShared) {
            if (! isset(static::$instances['locator'])) {
                static::$instances['locator'] = new FileLocatorCached(
                    new FileLocator(static::autoloader())
                );
            }

            return static::$mocks['locator'] ?? static::$instances['locator'];
        }

        return new FileLocator(static::autoloader());
    }
}

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.5 labels Oct 6, 2023
@kenjis kenjis marked this pull request as draft October 6, 2023 22:01
@kenjis kenjis force-pushed the feat-FileLocator-Caching branch 2 times, most recently from f890ccb to e109c6c Compare October 6, 2023 23:14
@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Oct 6, 2023
@lonnieezell
Copy link
Member

I think I started to do something very similar to this originally and found that PHP already does a great job of caching it's lookups so I didn't see any performance gains worth the memory usage.

Not sure if you're seeing differently now or not.

@kenjis
Copy link
Member Author

kenjis commented Oct 7, 2023

FileLocator does heavy jobs like to search all namespaces or read PHP class files and token_get_all() and find classnames.
So I expect performance gain in projects with many modules.
But I'm not sure this can improve the Welcome page benchmark.

I will benchmark this later.

@lonnieezell
Copy link
Member

I get that. I just remember that PHP's realpath_cache already did a job I wasn't able to improve on at the time. The framework has changed and does more things in different places then when I was first starting the rewrite, so maybe it will work better now.

And, personally, if I were working on optimizations, I would be more concerned about real-world performance then the hello world benchmark. But I would definitely say benchmarks should be ran before and after optimizations to make sure then actually work and don't just complicate the code.

@kenjis
Copy link
Member Author

kenjis commented Oct 7, 2023

CI4 is 5 to 7 times slower than CI3 on Hello World benchmark. #6889 (comment)
It is so slow that I would like to improve it, but it seems impossible to get the same level of performance
with optimization like caching I sent PRs. We need some kind of breakthrough.

Yes, you are probably right that tuning in real world applications is more important. However, in many cases, I think it will be tuning the interaction with the database.

Maybe there is room for tuning around the Model/Database library, but I'm not sure.

@kenjis
Copy link
Member Author

kenjis commented Oct 7, 2023

Ubuntu 22.04, PHP Version 8.1.2-1ubuntu2.14, Apache.

(4.5 *$=)$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     7.22ms    3.42ms  45.07ms   84.27%
    Req/Sec   142.91     31.66   202.00     57.00%
  7131 requests in 5.02s, 122.57MB read
Requests/sec:   1420.20
Transfer/sec:     24.41MB
(feat-FileLocator-Caching *$=)$ 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.09ms    2.57ms  24.67ms   83.53%
    Req/Sec   166.74     22.79   222.00     61.00%
  8317 requests in 5.01s, 142.91MB read
Requests/sec:   1659.92
Transfer/sec:     28.52MB

@lonnieezell
Copy link
Member

Oh nice! I was just letting you know what I ran into when I tried doing that early on. Awesome to see it helped this time.

And I'm glad to see someone tackling performance. I apologize if it came across disparaging.

@kenjis kenjis force-pushed the feat-FileLocator-Caching branch from e109c6c to a86e405 Compare October 8, 2023 06:15
@kenjis kenjis removed the breaking change Pull requests that may break existing functionalities label Oct 8, 2023
@kenjis
Copy link
Member Author

kenjis commented Oct 8, 2023

Change the implementation a bit.

  • No breaking changes. No changes at all when not using caching.
  • Caches all public method results.

Ubuntu 22.04, PHP Version 8.1.2-1ubuntu2.14, Apache (mod_php).

(4.5 *$=)$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     7.24ms    3.46ms  36.01ms   85.21%
    Req/Sec   142.62     33.53   191.00     58.40%
  7114 requests in 5.01s, 122.27MB read
Requests/sec:   1419.47
Transfer/sec:     24.40MB
(feat-FileLocator-Caching *$=)$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.73ms    2.40ms  33.09ms   87.48%
    Req/Sec   219.71     53.40   292.00     56.60%
  10951 requests in 5.01s, 188.16MB read
Requests/sec:   2186.65
Transfer/sec:     37.57MB

Prepare:

$ rm .env
$ composer update --no-dev

@kenjis kenjis force-pushed the feat-FileLocator-Caching branch 2 times, most recently from df4920c to e57d996 Compare October 8, 2023 08:33
@kenjis
Copy link
Member Author

kenjis commented Oct 8, 2023

This is off topic, but I got 5210 rps that is about 2 times slower than CI3 on Hello World benchmark.
See also #6889 (comment)

  • Config caching
  • FileLocator caching
  • Preload
  • Change the parent class of Services
$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.04ms    1.40ms  21.57ms   91.99%
    Req/Sec   531.46    109.34     1.08k    66.14%
  26575 requests in 5.10s, 5.38MB read
Requests/sec:   5210.70
Transfer/sec:      1.05MB
Configuration
diff --git a/app/Config/Services.php b/app/Config/Services.php
index df7c8ad086..097876274c 100644
--- a/app/Config/Services.php
+++ b/app/Config/Services.php
@@ -2,31 +2,21 @@
 
 namespace Config;
 
-use CodeIgniter\Config\BaseService;
+use CodeIgniter\Autoloader\FileLocator;
+use CodeIgniter\Autoloader\FileLocatorCached;
 
-/**
- * Services Configuration file.
- *
- * Services are simply other classes/libraries that the system uses
- * to do its job. This is used by CodeIgniter to allow the core of the
- * framework to be swapped out easily without affecting the usage within
- * the rest of your application.
- *
- * This file holds any application-specific services, or service overrides
- * that you might need. An example has been included with the general
- * method format you should use for your service methods. For more examples,
- * see the core Services file at system/Config/Services.php.
- */
-class Services extends BaseService
+class Services extends \CodeIgniter\Config\Services
 {
-    /*
-     * public static function example($getShared = true)
-     * {
-     *     if ($getShared) {
-     *         return static::getSharedInstance('example');
-     *     }
-     *
-     *     return new \CodeIgniter\Example();
-     * }
-     */
+    public static function locator(bool $getShared = true)
+    {
+        if ($getShared) {
+            if (! isset(static::$instances['locator'])) {
+                static::$instances['locator'] = new FileLocatorCached(new FileLocator(static::autoloader()));
+            }
+
+            return static::$mocks['locator'] ?? static::$instances['locator'];
+        }
+
+        return new FileLocator(static::autoloader());
+    }
 }
diff --git a/preload.php b/preload.php
index 63c781c220..6a1a214747 100644
--- a/preload.php
+++ b/preload.php
@@ -49,7 +49,7 @@ class preload
      */
     private array $paths = [
         [
-            'include' => __DIR__ . '/vendor/codeigniter4/framework/system',
+            'include' => __DIR__ . '/system',
             'exclude' => [
                 // Not needed if you don't use them.
                 '/system/Database/OCI8/',
diff --git a/public/index.php b/public/index.php
index 826966179e..4358b5132e 100644
--- 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

@kenjis kenjis marked this pull request as ready for review October 8, 2023 10:26
@lonnieezell
Copy link
Member

that's a great improvement!

One thing I think it needs, though, is the ability to enable/disable it. IMHO I think it should be disabled by default in development, but enabled in other environments. Otherwise developing with this in place sounds like a pain having to constantly clear the cache, unless I missed something.

@github-actions github-actions bot added the stale Pull requests with conflicts label Oct 11, 2023
@github-actions
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the feat-FileLocator-Caching branch from e57d996 to b62808e Compare October 11, 2023 05:00
@kenjis kenjis removed the stale Pull requests with conflicts label Oct 11, 2023
@kenjis
Copy link
Member Author

kenjis commented Oct 11, 2023

I have created a new class FileLocatorCached with cache functionality, so caching is disabled by default.

If we clear the cache when we deploy, there is no problem, but I don't think such a practice is common among CodeIgniter users. So I'm not sure if this caching should be enabled by default in production environment.

@github-actions github-actions bot added the stale Pull requests with conflicts label Oct 12, 2023
@github-actions
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

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.

I like the concept and execution, not a fan of the architecture. I understand why you did it, but I would rather see you introduce an interface than mix composition and inheritance of the same class. I expect switching Services to the interface would have very low likelihood of project disruption (which is already covered by 4.5) since this seems a pretty unlikely class for someone to replace.

@MGatner
Copy link
Member

MGatner commented Oct 12, 2023

environments

One thing we lose from service location instead of a container is an easy ability to provide environment-specific dependencies. Maybe we should be leveraging our Boot folder more to inject some of these, or include a .env-style config for Services. We could seed app/Config/Services.php with some recommended defaults but then devs who don't patch will just diverge more.

@kenjis kenjis force-pushed the feat-FileLocator-Caching branch from b62808e to 6b3cdab Compare October 13, 2023 01:13
@kenjis kenjis added breaking change Pull requests that may break existing functionalities and removed stale Pull requests with conflicts labels Oct 13, 2023
@kenjis kenjis force-pushed the feat-FileLocator-Caching branch from 1654fd0 to 0cdc5af Compare October 13, 2023 01:52
@kenjis
Copy link
Member Author

kenjis commented Oct 13, 2023

@MGatner Added a new Interface. But it brought breaking changes.

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.

Thank you. I think these changes are acceptable for a minor release, where we are already expecting devs who extend core classes to be attentive to breaking changes. I think it's very unlikely that anyone has extended FileLocator, and the swap to interfaces elsewhere should be trivial.

@kenjis kenjis merged commit a4bc7c0 into codeigniter4:4.5 Oct 16, 2023
53 checks passed
@kenjis kenjis deleted the feat-FileLocator-Caching branch October 16, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants