-
Notifications
You must be signed in to change notification settings - Fork 7
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
Merge duplicate array loops #208
Conversation
@mariushoch @JeroenDeDauw I suggest to merge at least this (and possibly #207) before tagging the new release (which is already prepared via #209). |
Benchmarked locally. Performance gain is a bit more than 3%. |
3% when just calling the deserialization code? What about in any real world use case where the data needs to be read from somewhere? I imagine it is no longer significant then |
Even if it's only 1% in a real world scenario, 1% equals 20 minutes, according to https://phabricator.wikimedia.org/T157013#2993060. Under these circumstances I don't think it's worth arguing what the value of duplicate code is. |
@@ -52,6 +54,10 @@ private function getDeserialized( array $serialization ) { | |||
|
|||
foreach ( $serialization as $key => $statementArray ) { | |||
if ( is_string( $key ) ) { | |||
if ( !is_array( $statementArray ) ) { | |||
throw new DeserializationException( 'The statements per property should be an array' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to mention key in the message
@@ -52,6 +54,10 @@ private function getDeserialized( array $serialization ) { | |||
|
|||
foreach ( $serialization as $key => $snakArray ) { | |||
if ( is_string( $key ) ) { | |||
if ( !is_array( $snakArray ) ) { | |||
throw new DeserializationException( 'The snaks per property should be an array' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to mention key here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both cases are old exception messages, and I did not wanted to change them, but I can.
3a25cb9
to
ec3c5f8
Compare
Well is it even 1%? If it's 3% just when executing the PHP code I expect it to be way less than 1% when holding i/o into account. |
+1 |
This patch does have two effects:
is_string
.This is a direct follow up for #205.
Bug: T157959