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

__callStatic: cache created Enum instances #129

Merged
merged 3 commits into from
Nov 14, 2020

Conversation

danielbeardsley
Copy link
Contributor

Instead of creating a new one each time, cache them.

This only matters if you're creating thousands of these objects.

I've used a small test script:

<?
require './vendor/autoload.php';

class Fast extends MyCLabs\Enum\Enum {
   protected const VALUE_ONE = "one";
   protected const VALUE_TWO = "two";
}

$t = microtime(true);
for ($i = 0; $i < 1000; $i++) {
   Fast::VALUE_ONE();
}
$secondsPerOp = (microtime(true) - $t) / 1000;
var_dump("Microseconds per op: " . $secondsPerOp * 1000000);

Before this change, we were getting about 4us per operation,
after this, we get about 1us per op, a 4X speedup!!

Note: This only matters if you end up constructing thousands of these
objects. We end up using this library extensively and thus end up
incurring more than ten milliseconds per request just for Enum
construction.

Instead of creating a new one each time, cache them.

This only matters if you're creating thousands of these objects.

I've used a small test script:

    <?
    require './vendor/autoload.php';

    class Fast extends MyCLabs\Enum\Enum {
       protected const VALUE_ONE = "one";
       protected const VALUE_TWO = "two";
    }

    $t = microtime(true);
    for ($i = 0; $i < 1000; $i++) {
       Fast::VALUE_ONE();
    }
    $secondsPerOp = (microtime(true) - $t) / 1000;
    var_dump("Microseconds per op: " . $secondsPerOp * 1000000);

Before this change, we were getting about 4us per operation,
after this, we get about 1us per op, a 4X speedup!!

Note: This only matters if you end up constrcuting thousands of these
objects. We end up using this library extensively and thus end up
incurring more than ten milliseconds per request just for Enum
construction.
src/Enum.php Show resolved Hide resolved
@danielbeardsley
Copy link
Contributor Author

Not sure why travis isn't enabled on this repo... there's a travis.yml config. Either way, passes 100%

image

@willemstuursma
Copy link
Contributor

Maybe it is good to return a clone of the cached instance, so no one will accidentally use === to compare enums.

Enums shouldn't be compared with ===, but static accessors are now
cached so === was working. Let's return a clone each time so === won't
be true for values retrieved from these accessors.

Note: this actually had no impact on the performance gains from a few
commits ago.
@danielbeardsley
Copy link
Contributor Author

so no one will accidentally use === to compare enums

That's a good point. People might accidentally start using that if they are literally the same instances.

Pushed a commit.

Performance gains still intact:
image

@willemstuursma
Copy link
Contributor

LGTM, for what it's worth @mnapoli.

@mnapoli
Copy link
Member

mnapoli commented Nov 14, 2020

Let's do it, thanks all!

@mnapoli mnapoli merged commit d178027 into myclabs:master Nov 14, 2020
@rodion-k
Copy link

Maybe it is good to return a clone of the cached instance, so no one will accidentally use === to compare enums.

Why is it bad to use === to compare enums?

@danielbeardsley
Copy link
Contributor Author

Because your code would then functionally depend on caching to be correct. If caching changed, your code would break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants