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

perf: autoloader #8005

Merged
merged 16 commits into from
Oct 11, 2023
Merged

perf: autoloader #8005

merged 16 commits into from
Oct 11, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Oct 4, 2023

Description
To improve the performance of autoloader.
Benchmark: #8005 (comment) and #8005 (comment)

  • add App and Config namespaces to composer.json
  • change the priority of the autoloaders
    Before: 1. CI classmap loader, 2. CI PSR-4 autoloader, 3. Composer autoloader
    After: 1. Composer autoloader, 2. CI classmap loader, 3. CI PSR-4 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

We can't customize `Config` namespace name.
It is set in Config\Autoload, and it may be renamed.
- When we use Composer, we use Composer autoloader first.
  So no need to load Composer classmap to CI4 autoloader.
- Add `App` and `Config` namespaces to composer.json.
  So we can use composer dump-autoload for app classes.
@kenjis kenjis added the 4.5 label Oct 4, 2023
1) CodeIgniter\Config\ConfigTest::testCreateNonConfig
ErrorException: Constant EVENT_PRIORITY_LOW already defined

/home/runner/work/CodeIgniter4/CodeIgniter4/app/Config/Constants.php:84
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/Factories.php:271
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/Factories.php:148
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/Config.php:31
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/ConfigTest.php:51
@lonnieezell
Copy link
Member

Have you done benchmarks on this? Seems like it would be about equivalent.

Also feel like it might cause problems with things like namespaces views, but it's been awhile since I've been in that code.

I feel like we'd get more performance from simply limiting the namespaces we scan with composer which we can already do.

@lonnieezell
Copy link
Member

One additional thought - more of a historical perspective, I suppose: when I first started the CI4 rewrite one of the goals was to make it not need Composer so that - much like in CI3 - you could run with or without Composer on your system. Though I may have killed that idea once I added in Kint lol.

Maybe it's time to get rid of our local autoloader and rely primarily on Composer in v5? Not sure. We're also not working on 5 currently so a moot point I guess.

@kenjis
Copy link
Member Author

kenjis commented Oct 4, 2023

The performance is clearly improved in the Welcome page benchmark.

(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    13.58ms    5.49ms  41.51ms   79.69%
    Req/Sec    74.40     19.25   190.00     65.67%
  3726 requests in 5.10s, 64.03MB read
Requests/sec:    730.82
Transfer/sec:     12.56MB
(perf-autoloader *$=)$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    12.81ms    5.24ms  48.98ms   79.15%
    Req/Sec    79.02     20.77   171.00     51.98%
  3979 requests in 5.10s, 68.38MB read
Requests/sec:    780.17
Transfer/sec:     13.41MB

Ubuntu 22.04 + Apache + mod_php

PHP 8.1.2-1ubuntu2.14 (cli) (built: Aug 18 2023 11:41:11) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2-1ubuntu2.14, Copyright (c), by Zend Technologies
    with Xdebug v3.1.2, Copyright (c) 2002-2021, by Derick Rethans
$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"

@kenjis
Copy link
Member Author

kenjis commented Oct 4, 2023

I feel like we'd get more performance from simply limiting the namespaces we scan with composer which we can already do.

It is true if there are a lot of namespaces, but
there are only two packages by default.

$ composer show
laminas/laminas-escaper 2.12.0 Securely and safely escape HTML, HTML attributes, JavaScript, CSS, and URLs
psr/log                 2.0.0  Common interface for logging libraries
$ php spark namespaces

CodeIgniter v4.4.1 Command Line Tool - Server Time: 2023-10-04 21:14:00 UTC+00:00

+-----------------+----------------------------------------+--------+
| Namespace       | Path                                   | Found? |
+-----------------+----------------------------------------+--------+
| CodeIgniter     | ROOTPATH/system                        | Yes    |
| Config          | APPPATH/Config                         | Yes    |
| App             | ROOTPATH/app                           | Yes    |
| Psr\Log         | VENDORPATH/psr/log/src                 | Yes    |
| Laminas\Escaper | VENDORPATH/laminas/laminas-escaper/src | Yes    |
+-----------------+----------------------------------------+--------+

@kenjis
Copy link
Member Author

kenjis commented Oct 4, 2023

Restricting the number of packages to be scanned raised the numbers a bit, but I are not sure if this is a significant difference.

https://codeigniter4.github.io/CodeIgniter4/general/modules.html#specify-composer-packages

--- a/app/Config/Modules.php
+++ b/app/Config/Modules.php
@@ -60,7 +60,9 @@ class Modules extends BaseModules
      *
      * @var array
      */
-    public $composerPackages = [];
+    public $composerPackages = [
+        'only' => [],
+    ];
 
     /**
      * --------------------------------------------------------------------------
(perf-autoloader *$=)$ wrk -t10 -d5s http://localhost/CodeIgniter4/
Running 5s test @ http://localhost/CodeIgniter4/
  10 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    12.83ms    5.30ms  52.28ms   79.99%
    Req/Sec    78.95     20.03   120.00     52.00%
  3945 requests in 5.02s, 67.78MB read
Requests/sec:    785.26
Transfer/sec:     13.49MB

@kenjis
Copy link
Member Author

kenjis commented Oct 4, 2023

more of a historical perspective, I suppose: when I first started the CI4 rewrite one of the goals was to make it not need Composer so that - much like in CI3 - you could run with or without Composer on your system. Though I may have killed that idea once I added in Kint lol.

It is not killed because the Zip installation still works.

This PR is to give priority to Composer's autoloader when using Composer.

Composer's autoloader already has a dump of the class map, and there is no need to copy it to CI's autoloader and use CI's autoloader.

@lonnieezell
Copy link
Member

Thanks for doing those numbers. It had a more significant improvement than I expected.

@kenjis
Copy link
Member Author

kenjis commented Oct 4, 2023

The results without Xdebug.

(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.14ms    3.18ms  30.54ms   81.33%
    Req/Sec   143.33     33.73   191.00     56.00%
  7148 requests in 5.01s, 122.84MB read
Requests/sec:   1426.96
Transfer/sec:     24.52MB
(perf-autoloader *$=)$ 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.53ms    3.07ms  31.12ms   83.13%
    Req/Sec   157.75     38.70   240.00     55.00%
  7869 requests in 5.02s, 135.21MB read
Requests/sec:   1566.75
Transfer/sec:     26.92MB

@kenjis
Copy link
Member Author

kenjis commented Oct 4, 2023

4.5 with Composer:

(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.14ms    3.31ms  36.40ms   81.77%
    Req/Sec   143.93     35.01   280.00     56.94%
  7239 requests in 5.10s, 124.42MB read
Requests/sec:   1419.50
Transfer/sec:     24.40MB

4.5 without Composer (removed vendor/):

(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.03ms    3.30ms  38.42ms   82.78%
    Req/Sec   146.29     35.54   380.00     64.67%
  7312 requests in 5.10s, 125.64MB read
Requests/sec:   1434.11
Transfer/sec:     24.64MB

@kenjis kenjis force-pushed the perf-autoloader branch 2 times, most recently from 0fd30c5 to d04c5c9 Compare October 5, 2023 00:15
@kenjis
Copy link
Member Author

kenjis commented Oct 5, 2023

The App namespace name might be changed by devs.
So we cannot put it in CI4's composer.json.
Removed.

Copy link
Collaborator

@ddevsr ddevsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenjis
Copy link
Member Author

kenjis commented Oct 5, 2023

Added to appstarter composer.json, and updated the docs.

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Oct 6, 2023
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.

Nice improvement, and great docs.

@kenjis kenjis merged commit b6684e6 into codeigniter4:4.5 Oct 11, 2023
55 checks passed
@kenjis kenjis deleted the perf-autoloader branch October 11, 2023 04:02
@kenjis kenjis mentioned this pull request Feb 22, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants