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 Services::get() #8607

Merged
merged 9 commits into from
Mar 7, 2024
Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Mar 4, 2024

Description

  • add Services::get() method to get the shared instance fast.
  • service() uses Services::get()
 Test 			Time 	Memory
services::timer() 	0.1337 	0 Bytes
services::get('timer') 	0.0406 	0 Bytes
service('timer') 	0.0528 	0 Bytes
<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;
use Config\Services;

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

        Services::timer();

        $iterator->add('Services::timer()', static function () {
            $timer = Services::timer();
        });

        $iterator->add("Services::get('timer')", static function () {
            $timer = Services::get('timer');
        });

        $iterator->add("service('timer')", static function () {
            $timer = service('timer');
        });

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

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 refactor Pull requests that refactor code 4.5 labels Mar 4, 2024
*
* @return mixed Entry.
*/
public static function get(string $key): mixed
Copy link
Member

Choose a reason for hiding this comment

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

Applicable review until override().

mixed should be limited to object, as the static array prop $instances accepts object as value. I don't think we need to accept anything to be set. Accepting mixed may lead to future misuses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to ?object.

* Sets an entry.
*
* @param string $key Identifier of the entry.
* @param mixed $value Normally an object.
Copy link
Member

Choose a reason for hiding this comment

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

See comment before.

public static function set(string $key, mixed $value): void
{
if (isset(static::$instances[$key])) {
throw new InvalidArgumentException('The Entry for "' . $key . '" is already set.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new InvalidArgumentException('The Entry for "' . $key . '" is already set.');
throw new InvalidArgumentException('The entry for "' . $key . '" is already set.');

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

* Overrides an existing entry.
*
* @param string $key Identifier of the entry.
* @param mixed $value Normally an object.
Copy link
Member

Choose a reason for hiding this comment

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

See comment above

@kenjis kenjis merged commit 452543e into codeigniter4:4.5 Mar 7, 2024
46 checks passed
@kenjis kenjis deleted the feat-Services-get branch March 7, 2024 07:35
@lonnieezell
Copy link
Member

The more I think about this the more confused I am about the performance issue we're seeing. All methods are static and should be called before __callStatic if the case matches, shouldn't it? So that should be higher performance than either get() or __callStatic.

@kenjis
Copy link
Member Author

kenjis commented Mar 8, 2024

The current Services is not that simple.
__callStatic is called in most cases.
Because Config\Services extends BaseService.

class Services extends BaseService

and BaseService has only autoloader and locator.

$ grep function system/Config/BaseService.php 
    public static function get(string $key): ?object
    public static function set(string $key, object $value): void
    public static function override(string $key, object $value): void
    protected static function getSharedInstance(string $key, ...$params)
    public static function autoloader(bool $getShared = true)
    public static function locator(bool $getShared = true)
    public static function __callStatic(string $name, array $arguments)
    public static function serviceExists(string $name): ?string
    public static function reset(bool $initAutoloader = true)
    public static function resetSingle(string $name)
    public static function injectMock(string $name, $mock)
    protected static function buildServicesCache(): void

@lonnieezell
Copy link
Member

Yeah, I guess the only ones that would hit directly would be in app/Config/Services. That's too bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants