Optimize array_contains functions #1395
Conversation
Improve runtime performance on `array_contains()`, `array_scontains()` and `array_contains_ic()` by iterating over the array values whenever possible, rather than creating a keyset to iterate over and calling the getter to get all values.
| @@ -814,7 +814,7 @@ public Version since() { | |||
| @Override | |||
| public Iterator<Mixed> iterator() { | |||
| if(associativeMode) { | |||
| throw new RuntimeException("iterator() cannot be called on an associative array"); | |||
| return associativeArray.values().iterator(); | |||
There was a problem hiding this comment.
This change worries me. Inherently, it's not possible to generally say whether someone intended to iterate the keys or values when getting the iterator, and I think it's reasonable to assume the opposite of whichever way we choose here. Indeed, Java's Map doesn't directly implement Iterable for a reason (though it provides entrySet, values, etc, to make it easy to iterate in various ways). I know methodscript arrays are terribly overloaded, but I think trying to directly iterate on an associative array is a sign that you're doing something you shouldn't, and so this should remain an error. array_keys and array_normalize exist, allowing people to iterate in either way, albeit not particularly lazily. Eventually though, there will be a standard object library, which will include proper (and non-magical) maps, sets, etc, with proper iterators, but for now, I would prefer to keep this strict and not allow direct iteration on an associative array.
Improve runtime performance on
array_contains(),array_scontains()andarray_contains_ic()by iterating over the array values whenever possible, rather than creating a keyset to iterate over and calling the getter to get all values.Running the same code, profiler results (where the profiler started at different points in time) do show a large difference in
array_contains().Before:
After:
