Skip to content
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

Improve ConstantArrayType union performance #1176

Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Apr 4, 2022

This gets rid of some complicated code that repeatedly calls ConstantArrayType::setOffsetValueType which leads to a new creation of a ConstantArray via the builder which leads to union again and so on. Apparently ConstantArrayType::mergeWith, which is used via reduceArrays, does already what needs to be done.

There seems to be just one catch: The keys in constant arrays are ordered a bit different in some cases and re-ordering them via e.g. UnionTypeHelper::sortTypes is too time-intensive. But if only 3 very dynamic cases are affected I hope that should be fine.

This is a huuuge improvement for phpstan/phpstan#6948

But let's see what CI says first before I cheer too loud..

@herndlm herndlm force-pushed the improve-constantarray-union-performance branch from b127929 to 1151cc7 Compare April 4, 2022 22:48
@herndlm herndlm changed the base branch from 1.6.x to 1.5.x April 4, 2022 22:52
@herndlm herndlm changed the base branch from 1.5.x to 1.6.x April 4, 2022 22:53
@herndlm herndlm force-pushed the improve-constantarray-union-performance branch from 1151cc7 to b127929 Compare April 4, 2022 22:55
@herndlm herndlm changed the base branch from 1.6.x to 1.5.x April 4, 2022 22:56
@herndlm
Copy link
Contributor Author

herndlm commented Apr 4, 2022

OH YEAH 🎉

@staabm would be interesting if this improves your slow example too

I can try to get some numbers tomorrow.

@herndlm herndlm marked this pull request as ready for review April 4, 2022 23:10
@ondrejmirtes
Copy link
Member

Wow, awesome! :)

@ondrejmirtes ondrejmirtes merged commit 1cdd9ba into phpstan:1.5.x Apr 5, 2022
@ondrejmirtes
Copy link
Member

Thank you very much!

@herndlm herndlm deleted the improve-constantarray-union-performance branch April 5, 2022 07:25
@herndlm
Copy link
Contributor Author

herndlm commented Apr 5, 2022

while this is helping and it definitely makes sense to remove that code, there's still more to do to get better performance for really big nested arrays. E.g.

  • sorting them in UnionType is slow
  • calling setOffsetValueType via OffsetAccessAssignmentRule, OffsetAccessAssignOpRule and OffsetAccessValueAssignmentRule should maybe avoided if possible
  • and the biggest of all - TypeCombinator::reduceArrays which calls ConstantArrayType::mergeWith is the main culprit for the exponential cost and, in case of appending an array to itself, doesn't even reduce anything. that was not true

I'd like to still look more for general optimisations, but the point of introducing some kind of nesting limit and enforcing that for sorting, describing and reducing seems to be coming closer. OR, I really check explicitly if an array is appended to itself and react to that (by e.g. not reducing the types).

@staabm
Copy link
Contributor

staabm commented Apr 5, 2022

this change is awesome. less code in this critical path is a great thing.

would be interesting if this improves your slow example too

oh my. I have to apologize first..
the profiles I have posted recently were recordeded against a different version of phpstan I actually checked out.. please forget those.

sorry for that.


please find the latest profile against the latest phpstan 1.5.x commit here:

blackfire run --ignore-exit-status php ../phpstan-src-staabm/bin/phpstan analyze app/partner/controllers/clxAuftragController.php -vvv --debug --xdebug

grafik

grafik

https://blackfire.io/profiles/7b155139-a1c6-4d5b-95f2-7cc28fc393db/graph

it seems in my edge case file, phpstan is now taken most time in parsing.. 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants