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

Codechange: Use FindVehiclesWithOrder when removing a road stop. #12144

Merged
merged 1 commit into from Mar 28, 2024

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented Feb 20, 2024

Motivation / Problem

When removing a road stop tile, the road vehicle pool is iterated to find any road vehicles that are currently heading to the tile, so they can be rerouted. This can take time if there are a lot of vehicles.

Description

Road vehicles can only have the road stop tile as their destination if it is in their order lists, and searching shared order lists can be faster than searching the vehicle pool.

Search for vehicles with the road stop in their orders, and then reroute if their destination is the removed tile.

The change is inconsequential with most saves, however when road stops are removed in bulk (company bankrupt/reset) it can add up.

Save Master This PR
Wentbourne remove 1 road stop 28µs 6µs
Wentbourne reset_company 1 39,422µs 7,981µs
Xarick's 4kx4k remove 1 road stop 5,380µs 299µs
Xarick's 4kx4k stop_ai 2 7,531,394µs 418,717µs

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Searching shared orderlists is faster than searching the vehicle pool to find matching vehicles.
@LordAro
Copy link
Member

LordAro commented Feb 20, 2024

What happens when the number of orders is significantly larger than the list of vehicles? :)

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Feb 24, 2024
@PeterN
Copy link
Member Author

PeterN commented Feb 25, 2024

What happens when the number of orders is significantly larger than the list of vehicles? :)

Would a savegame with non-shared orders suffice? Or shall I fabric a savegame with 255 non-shared orders for every vehicle...

@PeterN
Copy link
Member Author

PeterN commented Feb 27, 2024

Yup, 5000 road vehicles with 255 unshared orders each, is considerably slower this way.

Master: 80µs
This PR: 3,675µs

@TrueBrain TrueBrain removed the backport requested This PR should be backport to current release (RC / stable) label Mar 1, 2024
@TrueBrain
Copy link
Member

@LordAro let's not backport these kind of changes moments before release. Feels like asking for trouble, don't you agree? :)

@michicc michicc merged commit 8746be8 into OpenTTD:master Mar 28, 2024
20 checks passed
PeterN added a commit to PeterN/OpenTTD that referenced this pull request Apr 21, 2024
OpenTTD#12144 replaced pool iteration with FindVehiclesWithOrder, however the test for current_order being OT_GOTO_STATION was erroneously removed.
@PeterN PeterN deleted the remove-roadstop-orderlist branch April 21, 2024 11:18
PeterN added a commit that referenced this pull request Apr 21, 2024
…12552)

#12144 replaced pool iteration with FindVehiclesWithOrder, however the test for current_order being OT_GOTO_STATION was erroneously removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants