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

Change: Group processing of vehicle ticks by type of vehicle. This allows use of PerformanceCounter instead of PerformanceAccumulator. #7248

Closed
wants to merge 1 commit into from

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented Feb 19, 2019

This gives a large performance benefit in the case of lot of vehicles, and I suspect is no worse even with not many vehicles.

The performance impact is caused by PerformanceAccumulator spending a disprortionate amount of time doing its own sums.

This fixes #7247.

…lows use of PerformanceCounter instead of PerformanceAccumulator.

This gives a large performance benefit in the case of lot of vehicles, and I suspect is no worse even with not many vehicles.
The performance impact is caused by PerformanceAccumulator spending a disprortionate amount of time doing its own sums.
@PeterN
Copy link
Member Author

PeterN commented Feb 19, 2019

With this change, performance in the Wentbourne save jumps 50% from around 8fps to 12fps.

Here a screenshot from ProZone 13 save:

splitticks

And Wentbourne:

splitticks3

@nielsmh nielsmh added this to the 1.10.0 milestone Feb 27, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Mar 3, 2019

Is this ready for review?

@PeterN
Copy link
Member Author

PeterN commented Mar 3, 2019

I'm not sure, it's kind of a brute force attempt, and it's been reported that the PerformanceAccumulator is not slow on some platforms, in which case iterating FOR_ALL_VEHICLES several times is actually slower.

In which case, I think an alternative approach using pre-built lists for each vehicle type would be preferable. These would only need to be maintained on saveload and create/destroy of vehicles.

@michicc
Copy link
Member

michicc commented Apr 14, 2019

Seeing that there seems to be repeated reports about this (Most recent one: https://www.tt-forums.net/viewtopic.php?p=1220451#p1220451), I think that this is a change that should be done. Also, de-tangling the vehicle loop might be a tiny first step towards doing some calculations in parallel.

Implementation-wise, having pre-sorted lists is probably useful (heck, they might even be useful in other places in the code).

@nielsmh nielsmh modified the milestones: 1.10.0, 1.11.0 Feb 9, 2020
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
@TrueBrain
Copy link
Member

I see an agreement in the comments that another approach over this PR would be the right way forward. I assume this PR will not receive any further updates, and a new PR will be made with the other approach if/when someone creates that.

So for now, going to close this PR. Feel free to reopen if I misread the comments :)

@TrueBrain TrueBrain closed this Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

With lots of vehicles, PerformanceAccumulator has a large performance impact itself
4 participants