-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Zend Db Query Builder Optimisation #6541
Zend Db Query Builder Optimisation #6541
Conversation
we only care that the name has a position, not what is is, so don't waste resources looking it up
@@ -107,8 +107,7 @@ public function offsetSet($name, $value, $errata = null) | |||
} | |||
} elseif (is_string($name)) { | |||
// is a string: | |||
$currentNames = array_keys($this->data); | |||
$position = array_search($name, $currentNames, true); | |||
$position = isset($this->data[$name]); |
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.
isset
and array_search
have different return types - are you sure this is equivalent?
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.
$position
is only checked to see if it is explicity false (see line 117), which isset
will return if it's not set.
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.
What about the null
value?
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.
We can only hit this code if $name
is a string, at which point we are only checking if it's already been used or not. Null name values are handled differently as before.
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.
Ignore me, I know what you mean, yeah will need to change this to array_key_exists
.
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.
isset will return false if value is null which is not what we want. `array_key_exists` will still be faster than `array_search`
Switched to using |
Cherry-picked to master for release to 2.3.2, and merged to both master and develop. |
Found that the query builder was spending a long time building queries with large arrays in the where clause. After a little bit of profiling noticed that line 111 was been called a lot.
array_search
is an expensive lookup, especially as the return value wasn't been used beyond checking it wasn't false, so replaced with a less expensiveisset
.