Slight performance improvements for Dir::inventory()

[distantnative]

This PR …

Tried to fix #5126 - while the issue itself remains without a good solution, some improvements to the methods structure and performance could be made. Added those in this PR.

Refactored

• Slight performance improvements for Dir::inventory()

Housekeeping

• Upgraded Psalm
• Added performance tests for Dir::inventory() with various number of page models

Ready?

• Unit tests for fixed bug/feature
• In-code documentation (wherever needed)
• Tests and checks all pass

For review team

• Add changes to release notes draft in Notion

[bastianallgeier]

Are there any before/after numbers to compare this or is there a good way for me to compare locally?

[distantnative]

@bastianallgeier

To run it yourself, you need to copy the added performance tests from bench/Filesystem over to the develop/minor state. There run composer run-script bench:baseline. Then you can switch back to the branch and run composer run-script bench. That way, performance of develop/minor is compared against this PR.

However, for me the result seems to be worse now after 7d97311

[bastianallgeier]

That's so weird. Do you have any idea why this could perform worse?

[distantnative]

@bastianallgeier I have reverted c903e6b again.

With it, the performance was worse than develop-minor, particularly with no or one model:

After reverting that change, this PR shows slight improvements over develop-minor:

My guess would be that for multilang installations, the difference isn't bad. But in the bench tests with single lang, calling the app instance even with $multilang = false adds quite some overhead.

[lukasbestle]

My guess would be that for multilang installations, the difference isn't bad. But in the bench tests with single lang, calling the app instance even with $multilang = false adds quite some overhead.

Can't we also use if ($multilang === true) when fetching the language code? If it's false, we don't even need to load the app object. $languageCode can just always be null then.

[lukasbestle]

Love the refactoring. Besides performance, the new structure makes much more sense and is easier to read and understand IMO.

[distantnative]

Can't we also use if ($multilang === true) when fetching the language code? If it's false, we don't even need to load the app object. $languageCode can just always be null then.

I think it could still have performance downsides for multilang inventories without children (currently app instance wouldn't be called then at all, after the change it would be called).