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. |
| 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.
Would be nice to mention key in the message
| 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.
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