Improve getKey method performance #136
Conversation
|
Nice one. |
tests/EnumTest.php
Outdated
| $bin = '4f3a33303a224d79434c6162735c54657374735c456e756d5c456e756d4669787'. | ||
| '4757265223a313a7b733a383a22002a0076616c7565223b733a333a22666f6f223b7d'; | ||
| $bin = '4f3a33303a224d79434c6162735c54657374735c456e756d5c456e756d46697874757265223a323a7b733a38' . | ||
| '3a22002a0076616c7565223b733a333a22666f6f223b733a363a22002a006b6579223b733a333a22464f4f223b7d'; |
There was a problem hiding this comment.
Ha, so this test was added to avoid such BC breaks.
I'm afraid this could be a deal breaker. In short, already serialized enums may not be compatible with the new class. Is that right?
There was a problem hiding this comment.
Yes, was thinking about it as well and wanted to test it further to make sure.
I'm guessing the key will be null (https://3v4l.org/lILJ5) and a fallback will work to be implemented.
The second issue I'm not sure is that serialization will be bigger in terms of bytes. What do you think about that?
There was a problem hiding this comment.
The longer serialized string isn't a bc break to me, we never promised anything on the length of that string.
And right, if I understand correctly what you say we need a fallback for the null value.
There was a problem hiding this comment.
I think the test should have some comments then to make it more clear what it is for. It looked a lot like a change detector test.
Maybe a data provider dpSerializedStringThatCanBeUnserialzed.
There was a problem hiding this comment.
In case we have to deal with unserializing Enums that doesn't have a key, I moved the logic in __wakeup().
I kept the unserializing test using the old format, adding equal comparison.
Let me know if that looks fine.
drealecs
force-pushed
the
improve_getKey_performance
branch
from
c436460 to
a0076be
Compare
… One can programatically call `isValid()` returning bool or just create a new instance with `from()` throwing exception
drealecs
force-pushed
the
improve_getKey_performance
branch
from
a0076be to
edcc10d
Compare
src/Enum.php
Outdated
| @@ -199,11 +205,13 @@ public static function isValid($value) | |||
| * @psalm-pure | |||
| * @psalm-assert T $value | |||
| */ | |||
| public static function assertValidValue($value): void | |||
| private static function assertValidValue($value): string | |||
There was a problem hiding this comment.
I propose to make private the newly public static method added in #135 as there was no new release so far.
I believe there is no need for have another public method as:
- If a user wants to programmatically test that a value is valid, it can do this by using
isValid()static method. - During instantiation,
from()can be used to have an exception thrown for invalid values.
I want to easily re-use the method to return the key when a value is valid for further storing in constructor.
There was a problem hiding this comment.
@michaelpetri, let me know if this makes sense and if you envision any other use-case that I missed
There was a problem hiding this comment.
I worked around it for now but did a change in from to optimize the construction.
There was a problem hiding this comment.
Sorry for my late response. imo no need to have it public since i just can call from($mixed) and get an enum or exception.
There was a problem hiding this comment.
Sorry missed the return string part, imo the assert should not return anything. If no exception was thrown you know you have a valid key, or do i miss something?
There was a problem hiding this comment.
The current version of the PR is with an additional private method called assertValidValueReturningKey() that returns string and having the current assertValidValue() still there calling it.
The operation does a full array scan and I would like to reuse the same array scan for retrieving the key in one operation, changing in_array with array_search.
assertValidValueReturningKey() would be called in ctor and in from() method as well.
There was a problem hiding this comment.
Okay, now I understand what you want to achieve. From my side it's ok, even if I think it makes the codebase a bit more confusing. Would another static map (value => key) be an option here as well? Then you could lookup keys by self::$otherMap[$value] ?? null ?
There was a problem hiding this comment.
That would not work as values can be anything, not just int or string.
There was a problem hiding this comment.
Okay, so nothing more from my side. 🤐
| $this->expectException(\UnexpectedValueException::class); | ||
| $this->expectExceptionMessage("Value 'foo' is not part of the enum " . EnumFixture::class); | ||
|
|
||
| EnumFixture::from(EnumFixture::FOO()); |
There was a problem hiding this comment.
I added one more test here to clearly mention from() should not work with the enum object itself, even if the exception text is unclear that 'foo' there is the actual object casted to string.
…without calling the constructor again
|
That's beautiful, thank you! |
Few months ago when I profile an application during some performance tests, I found that using
getKey()was taking more CPU than expected and switching togetValue()for that case solved the issue; it was for logging.For that specific case, the enum value object wasn't recreated but
getKey()was called multiple times on the duration of a script.In this change, the key is stored in constructor using the same cost basically.
I added to the branch the commit of @simPod from #123 as the changes there was something I also changed almost the same so he could have the credits.