[performance] Use static variables inside every singleton constructors.

[gnutix]

Hello folks,

Here's another performance optimization when static constructors are called a lot. It was already done for some methods in the library, so I kept the same code style and applied it to all static constructors that take no arguments ("constants", in a way).

Let me know if you think some of these do not deserve it. I applied it everywhere without much thinking about it, because I don't think there's any downside of having that static variable, and I definitely know that some of these methods show up a lot in our benchmarks, like TimeZone(Offset)::utc() and Duration::zero(), min()/max(), etc, and then it's definitely beneficial.

[gnutix]

I don't think it make sense to try and fix the code coverage for a static variable check, but if you think so, please let me know how and I can make the changes.

[BenMorel]

Hi,

I don't think DayOfWeek and Month will benefit much from this, as they call get() internally, which already caches instances.

Plus, in the next version (0.6), these classes will be converted to PHP 8.1 enums, so it won't be long before you get a much bigger improvement.

I agree with all the others, though!

I don't think it make sense to try and fix the code coverage for a static variable check, but if you think so, please let me know how and I can make the changes.

I still prefer to get it covered, so you can just add a test for each method that calls it twice and ensures that it returns the same instance!

[gnutix]

Tests added, reverted DayOfWeek and Month as suggested.

PS: i find it unexpected that ECS is configured with PhpCsFixer\Fixer\PhpUnit\PhpUnitTestCaseStaticMethodCallsFixer so that it forces using $this-> to call methods that are actually static (like assertSame). 🤷‍♂️

[BenMorel]

PS: i find it unexpected that ECS is configured with PhpCsFixer\Fixer\PhpUnit\PhpUnitTestCaseStaticMethodCallsFixer so that it forces using $this-> to call methods that are actually static (like assertSame). 🤷‍♂️

I tend to agree. @tigitz Do you want to change this?

[tigitz]

Yeah it's a matter of preference as explained here: https://docs.phpunit.de/en/10.3/assertions.html

The idea I guess from this rule would be to at least force consistency throughout the codebase.

But if you don't mind having both usage then I can remove it. Let me know.

@BenMorel Taking the opportunity to friendly remind you about the dedicated PR about CI and codestyle that is still pending your review 😉

[BenMorel]

Thank you, @gnutix!

@tigitz:

The idea I guess from this rule would be to at least force consistency throughout the codebase.

Definitely! I was thinking about switching to self, not allowing both. After double checking, this is actually the style I already use in brick/math and brick/money at least.

Taking the opportunity to friendly remind you about the dedicated PR about CI and codestyle that is still pending your review 😉

I know, I'm sorry, I'm still a bit unsure about whether this is the way I want things, so I postponed indefinitely 😕 But the lack of coding standards on the other libraries really bothers me, so I'll have to take a decision at some point.

In the meantime, if you want to change the PHPUnit code style, a PR is welcome 🙂

[solodkiy]

        /** @var Instant|null $epoch */
        static $epoch;

        if (!isset($epoch)) {
            $epoch = new Instant(0, 0);
        }

        return $epoch;

I think this variant is simpler, more cache-way

[gnutix]

Agree. That's how I spontaneously wrote the PR, then I changed it to match the current code style..

[BenMorel]

I don't mind if you want to change this.
If you do, please use if ($epoch === null) instead of isset(), which wouldn't alert on undefined variable!