FS#4934 - Profile - GetImage is slow

Attached to Project: OpenTTD
Opened by Patric Stout (TrueBrain) - Sunday, 01 January 2012, 20:37 GMT
Type Bug
Category Core
Status Confirmed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No


To centrally document:

One of the slowest functions atm is the GetImage function of Aircraft, Train and Road. Between 35% and 55% is spend in this function alone in savegames with many vehicles. This is a very high value.

GetImage on its turn calls GetCustomEngineSprite, which is around 50% of the CPU spend in GetImage function. Both functions appear hard to improve (taking between 100 and 500 instructions per call for Train::GetImage), but the main issue is in its frequency of calls. It is called many many many times. To give an idea of volume:

When 350k trains are updated, 10M times GetImage is called. 10M times GetImage takes on a random savegame 5.2B instructions to finish. The whole of Train::Tick (and all his children) take in this random savegame 10.1B instructions.
When 380k aircrafts are updated, 820k times GetImage is called. 820k times GetImage takes on the same random savegame 0.5B instructions to finish. The whole of AirCraft::Tick (and all his children) take in this savegame 1.3B instructions.
For road vehicles the impact is lower, mainly as the PathFinder plays a much bigger role there (which is another story). Ignoring the pathfinder, here too we see GetImage consuming a large amount of time.
Similar behaviour cannot be detected for ships.

To talk in averages: 1 train update costs ~15k instruction, of which 8k instructions are spend in GetImage.

All different ways to say: significant amounts are spend here.

Main question asked: can't we call GetImage only when we are drawing the vehicle?

On initial thoughts, it is most likely related to NewGRFs that it is updated this much. But being able to reduce the amount of calls would speed up large games by a good portion, so it might be worth the effort to find a solution. As I have very little knowledge of NewGRF, I made this bug report so someone else can hopefully pick this up and 'solve' the issue.
This task depends upon

Comment by frosch (frosch) - Tuesday, 03 January 2012, 23:41 GMT
I assume the most calls of GetImage() are via Vehicle::UpdateViewport().

This function does two things which need GetImage():
1) Update the position hash of vehicles with the new sprite size if the sprite changes. See VehicleUpdateViewport().
2) Redraw the vehicle if its sprite changes at some point.

About 1:
Maybe the exact size does not matter and VehicleUpdateViewport() can just use some default sprite dimension?

About 2:
Usually vehicle sprites only change when the vehicle move or load/unload. So, if we add enough Vehicle::MarkDirty() we might not need the check in Vehicle::UpdateViewport().

About calling GetImage() when drawing the vehicle:
This would dramatically affect drawing when zoomed out, wouldn't it? So, I guess Vehicle::cur_image is still needed for caching the result of GetImage(). So, if the sprite is known to change, one could set cur_image to INVALID_SPRITE or so, so mark the cache as invalid.
Comment by Peter Nelson (peter1138) - Tuesday, 22 January 2013, 13:40 GMT
Hacky patch to implement:
maximum sprite size caching (per vehicle per direction-ish)
update viewport area based on cached size
mark cur_image invalid
update cur_image with GetImage() when actually drawing the viewport

the lookup to determine maximum sprite size is horrible and slow. sprite cache needs to be cleared else it fills up.
aircraft shadows (and rotors?) are broken
some glitches with large sprites

second patch implements the same counters to compare number of calls of getcustomvehiclesprite() and drawviewport() without the changes.
quick test shows that calls to getcustomvehiclesprite() drop significantly on viewport extents as expected, but the viewport is drawn more often as a test which checks if the image has changed can no longer be performed.
Comment by Peter Nelson (peter1138) - Tuesday, 22 January 2013, 13:45 GMT
might be better to keep track of image size during newgrf load but that would waste space in the spritegroup structures (or require a lot of temp data)