Skip to content

Conversation

@danielbeardsley
Copy link
Member

@danielbeardsley danielbeardsley commented Oct 28, 2020

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.

@danielbeardsley
Copy link
Member Author

image

QA 👍

@danielbeardsley
Copy link
Member Author

@jarstelfox
Copy link

jarstelfox commented Oct 28, 2020

Does this pass psalm? Are you opening up a pull upstream?

@zdmitchell
Copy link

CR 🎸

@danielbeardsley
Copy link
Member Author

Does this pass psalm? Are you opening up a pull upstream?

Their whole repo doesn't pass psalm (this pull doesn't affect the number of errors). And yes, I will once we get CR here.

@jarstelfox
Copy link

jarstelfox commented Oct 28, 2020

Their whole repo doesn't pass psalm

What that's sad! I added it and made it pass 100% 😢

CR 🌵

@danielbeardsley
Copy link
Member Author

What that's sad! I added it and made it pass 100%

It's possible I'm wrong :-( Looking into it.

@jarstelfox
Copy link

jarstelfox commented Oct 28, 2020

Ah it looks like they don't use a composer.lock, psalm had an update. Got smarter. Started failing, no one cared. Lame. CC @danielbeardsley

@danielbeardsley
Copy link
Member Author

Master:

------------------------------
13 errors found
------------------------------

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.
@danielbeardsley
Copy link
Member Author

Amended the commit with psalm comment changes.

@jarstelfox
Copy link

CR 🌵

@danielbeardsley
Copy link
Member Author

QA 👍

image

@sterlinghirsh
Copy link
Member

CR ⚡

@jarstelfox
Copy link

CR 🌵

@danielbeardsley
Copy link
Member Author

We'll have to change our composer.json to use this repo as a dependency.

@sterlinghirsh
Copy link
Member

CR ⚡

@danielbeardsley danielbeardsley merged commit b516e25 into master Oct 29, 2020
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.

5 participants