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

Profile - GetImage is slow #4934

Closed
DorpsGek opened this issue Jan 1, 2012 · 4 comments
Closed

Profile - GetImage is slow #4934

DorpsGek opened this issue Jan 1, 2012 · 4 comments
Labels
enhancement Issue would be a good enhancement; we accept Pull Requests! flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) stale Stale issues

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Jan 1, 2012

TrueBrain opened the ticket and wrote:

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.

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/4934
@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 4, 2012

frosch wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4934#comment10669

@DorpsGek
Copy link
Member Author

peter1138 wrote:

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

Issues:
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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4934#comment11894

@DorpsGek
Copy link
Member Author

peter1138 wrote:

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)


This comment was imported from FlySpray: https://bugs.openttd.org/task/4934#comment11895

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
@TrueBrain TrueBrain added enhancement Issue would be a good enhancement; we accept Pull Requests! and removed bug from FlySpray labels Apr 14, 2018
@frosch123 frosch123 removed the Core label Apr 14, 2018
@andythenorth andythenorth added the stale Stale issues label Jan 5, 2019
@andythenorth
Copy link
Contributor

Although this would be nice to have, it isn't something we expect to fulfill in the next year, and on that basis I'm closing it. We do this to keep the project manageable, productive and fun. We hope you do understand. Thanks for contributing though! Here you can find more about how we handle feature requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue would be a good enhancement; we accept Pull Requests! flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) stale Stale issues
Projects
None yet
Development

No branches or pull requests

4 participants