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: add Factories::get() #8598

Closed
wants to merge 4 commits into from
Closed

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Mar 2, 2024

Description
See #6889 (comment)

  • add method to get the shared instance fast.

Benchmark

Test 			Time 	Memory
config() 		0.5684 	0 Bytes
factories::get() 	0.1305 	0 Bytes
<?php

namespace App\Controllers;

use CodeIgniter\Config\Factories;
use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new Iterator();

        $iterator->add('config()', static function () {
            $config = config('App');
        });

        $iterator->add('Factories::get()', static function () {
            $config = Factories::get('config', 'App');
        });

        return $iterator->run(30000);
    }
}

Profiling

Before:
Profile Factories before

After:
Profile Factories after

Environment

$ php -v
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

$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"

$ apache2 -v
Server version: Apache/2.4.52 (Ubuntu)
Server built:   2023-10-26T13:44:44

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 should use $options['component'] because it takes precedence.
See the code below to create a shared instance.
@kenjis kenjis marked this pull request as draft March 2, 2024 04:51
@kenjis kenjis added the 4.5 label Mar 2, 2024
@kenjis kenjis changed the title feat: add Factories::get() perf: add Factories::get() Mar 2, 2024
@kenjis kenjis force-pushed the feat-Factories-get branch 2 times, most recently from 05831b3 to 1b5fdb3 Compare March 2, 2024 05:25
@lonnieezell
Copy link
Member

Have you compared using the current config() method, with a version of it that uses Factory::get('config') to see the difference?

I find the helper method config() much more readable and pleasant and what I'd prefer to ask users to use if it makes sense. I'm assuming it speeds it up quite a bit since the big boost is the callStatic method in Factory, right? And as it stands, assuming the config file is used say 1000 times in a page load for an app, we're only looking at 0.000143 second increase with the proposed changes. While I think any performance boost is good, if the a modified config() call is close to the same speed I would prefer to stay with that.

@ddevsr
Copy link
Collaborator

ddevsr commented Mar 2, 2024

I am agree with @lonnieezell, we already introduces config() to get variable config using feature Factories release

@kenjis
Copy link
Member Author

kenjis commented Mar 2, 2024

Indeed, the performance improvement in the Welcome page benchmark in this PR was very small. The numbers are so varied that it is hard to tell if there is any real improvement.

However, the performance of Factories should have improved as seen in the profiles.

It might be certainly better to use config(), although there is the overhead of a function call.
That way, the config() implementation can be replaced by another container or something.

However, DI containers, service locators, whatever, they cannot make the framework faster. They can only slow it down. So even if we improve that, it seems impossible to keep up with CI3.

@kenjis kenjis mentioned this pull request Mar 2, 2024
5 tasks
@kenjis kenjis closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants