FS#4433 - Make code of realistic acceleration faster

Attached to Project: OpenTTD
Opened by Felix (rfc2795) - Friday, 21 January 2011, 23:19 GMT
Last edited by andythenorth (andythenorth) - Sunday, 13 August 2017, 17:00 GMT
Type Feature Request
Category Vehicles
Status Closed
Assigned To andythenorth (andythenorth)
Operating System All
Severity Low
Priority Normal
Reported Version 1.1.0-beta4
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No


in ground_vehicle.cpp file, function GetAcceleration line 130:
resistance += (area * this->gcache.cached_air_drag * speed * speed) / 1000;

1) I think, the calculation of speed * speed will be needed very often. So why don't cache this value in a array and substitut speed * speed with speed_square[speed]? (I hope this will be faster, because often is multiplication slower than other commands...). One the other hand a look up table will need a bit of ram.

2) Are area and gcache.air_drag both vehicle type specific constants? Thus why don't calculaten them one time? (same reason as before)
This task depends upon

Closed by  andythenorth (andythenorth)
Sunday, 13 August 2017, 17:00 GMT
Reason for closing:  Won't implement
Additional comments about closing:  Not a clear benefit, no sponsor, aging patch. Thanks, but closing.
Comment by Remko Bijker (Rubidium) - Saturday, 22 January 2011, 07:32 GMT
Most, if not all, opcode's durations are insignificant to cache misses. So replacing something with a large lookup table doesn't mean it magically becomes much faster. It also remains to be seen whether a quarter of a megabyte of RAM is still a bit or not.

For the second: yes, the area is somewhat constant (differs based on being in tunnel or not). The air drag is constant.

But for all optimisations the major amount of work isn't in thinking of possible optimisations, but implementing and, most importantly, validating that they are correct and optimisations without making the code totally unreadable.

So, what are your results in both patches and validation?
Comment by Felix (rfc2795) - Saturday, 22 January 2011, 13:35 GMT
Your right. To say somthing and to this, is one complett different levels.

My skills about programming c/c++ are a little rusty (and this of, how do make a patch, work with svn, etc. are equal to zero...). But I have implement the second point and I din't see a great impact. (I have testet with a savegame witch have over 500 trains)

I'm willing to do more, but only if the change have a positiv effect. So, can a other give me some feedback (good or bad is equal).
Comment by Zdeněk Sojka (SmatZ) - Saturday, 22 January 2011, 13:53 GMT
Rather than multiplication, I would be concerned about division. Luckily compiler optimised "n / 1000" to "(n * 274877907ULL) >> 38". Nevertheless, dividing by 1024 (and thus allowing >>10) would be far nicer. Also, you got a point about caching some values, especially multiplying two constants looks wrong. However, I don't think using a precomputed table for multiplication of speed**2 would help.
It would be nice to have this function profiled - to see how often it is called and how long does its execution take.
Comment by Felix (rfc2795) - Saturday, 22 January 2011, 16:49 GMT
Yes, division are complex operation. I has hestitat to change this, because it need caution how big the variable musst be (multiplication and division). I change it and now, the division won't be execute ervertime the GetAcceleration will be called. And I didn't see much of a "impact". So either, the compiler does a good job, I'm wrong with my idea, the code hasn't needed much cpu time before or all together.