Use foreach over slower functions array_map() and Utils::map()

[spawnia]

This increases the performance measured in the benchmarks by about 1-3% on average.

While there are some spots where the map functions may look a bit nicer, other parts were complicated quite a bit by its use (e.g. double array_map() into array_combine()).

[coveralls]

Coverage decreased (-0.03%) to 86.256% when pulling 872220b on spawnia:replace-utils-map-with-array-map into 5042bed on webonyx:master.

[vladar]

Did you have a chance to check how much it will improve performance?

My concern about this PR is that there are also other utility methods that will be harder to convert. Also, I mainly had this method for consistency of ordering of arguments of map, reduce, keys, etc. Without it other methods make way less sense.

[spawnia]

I lazily assumed it makes things faster, and am pretty sure it does.

It makes sense for me to have Utils in place when translating from JS. Once the conversion is done and a level of maturity is achieved, rewriting in idiomatic PHP and optimizing performance is a logical next step.

I think every PHP developer is familiar with foreach and array_map, so this should not impact readability in a negative way. We can tackle the other Utils later.

[vladar]

Another thing to keep in mind is that Utils::map supports both array and Traversable while array_map only works with arrays. So we must carefully check every array_map usage to make sure only arrays are possible as an input.

In general, I don't feel like this is a valuable change. I can be proven wrong with some benchmarks though.

[vladar]

I recall that part of the reason for using Utils::map was when we switched from arrays to NodeList in AST. In many cases, I just don't want to think about the type of iterable, which you kinda always have to do with array_map

[spawnia]

I just don't want to think about the type of iterable, which you kinda always have to do with array_map

I totally agree with that sentiment. However, in PHP this kind of flexibility comes with a runtime cost. Manual monomorphization can be tedious, but PHPStan and the test suite should have us covered here.

I will try and produce some benchmark results.

[spawnia]

Added a benchmark that does a lot of schema parsing, triggering code within ASTDefinitionBuilder. The foreach variant consistently performed better, around 1-3%.

I just don't want to think about the type of iterable, which you kinda always have to do with array_map

We could just convert everything to foreach, which is always the fastest.

I think that particularly in library code, such micro-optimizations do matter. Personal gusto or ease of writing becomes less important when you consider the (as of writing this) 6652 users that are currently wasting CPU cycles: https://github.com/webonyx/graphql-php/network/dependents?package_id=UGFja2FnZS01NDI5NjAyMjc%3D

[codecov]

Codecov Report

Merging #712 (d95d7d9) into master (ec34041) will increase coverage by 0.01%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master     #712      +/-   ##
==========================================
+ Coverage   94.12%   94.14%   +0.01%     
==========================================
  Files         117      117              
  Lines        9761     9715      -46     
==========================================
- Hits         9188     9146      -42     
+ Misses        573      569       -4     

Impacted Files	Coverage Δ
...c/Executor/Promise/Adapter/ReactPromiseAdapter.php	0.00% <0.00%> (ø)
src/Executor/ReferenceExecutor.php	95.04% <ø> (ø)
src/Language/DirectiveLocation.php	100.00% <ø> (ø)
src/Validator/Rules/QueryComplexity.php	92.74% <0.00%> (+1.47%)	⬆️
src/Type/Definition/ObjectType.php	98.11% <66.66%> (-1.89%)	⬇️
src/Type/SchemaValidationContext.php	96.00% <80.00%> (-0.20%)	⬇️
src/Error/Error.php	94.94% <90.24%> (+6.06%)	⬆️
src/Error/FormattedError.php	75.35% <90.90%> (-0.35%)	⬇️
src/Language/Source.php	100.00% <100.00%> (ø)
src/Server/Helper.php	82.66% <100.00%> (-0.16%)	⬇️
... and 17 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec34041...d95d7d9. Read the comment docs.

[simPod]

should I target some places where I think we can use map in extra branch so you can cherrypick?

[simPod]

is there a benefit from nullability somewhere?

I guess this would also classify as BC break

[spawnia]

Consistency across the other props here that can maybe hold an array. While an empty array is an acceptable default for most cases, I think the optionality of this is best expressed by null. We don't go around and add empty strings as defaults for optional strings everywhere either.

[spawnia]

should I target some places where I think we can use map in extra branch so you can cherrypick?

Sounds good. I would like to get this one merged if it is alright in order to limit the amount of WIP merge requests. I do plan to follow up with removal of other, expensive utils.

[simPod]

Aight, let's go 🚢 . I'll follow with something on top of this soon.