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

Make code of realistic acceleration faster #4433

Closed
DorpsGek opened this issue Jan 22, 2011 · 5 comments
Closed

Make code of realistic acceleration faster #4433

DorpsGek opened this issue Jan 22, 2011 · 5 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

rfc2795 opened the ticket and wrote:

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)

Reported version: 1.1.0-beta4
Operating system: All


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

Rubidium wrote:

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?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4433#comment9514

@DorpsGek
Copy link
Member Author

rfc2795 wrote:

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).

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4433#comment9518

@DorpsGek
Copy link
Member Author

SmatZ wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4433#comment9519

@DorpsGek
Copy link
Member Author

rfc2795 wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4433#comment9521

@DorpsGek
Copy link
Member Author

andythenorth closed the ticket.

Reason for closing: Won't implement

Not a clear benefit, no sponsor, aging patch. Thanks, but closing.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4433

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) Vehicles labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

1 participant