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

Feature: NewGRF callback profiling #7868

Merged
merged 16 commits into from Jan 26, 2020
Merged

Conversation

nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Dec 22, 2019

Adds a console command newgrf_profile to collect some profiling data about NewGRF action 2 callbacks and produce a CSV file.

I'm not entirely sure if this is useful or not, hopefully someone can try it out and decide. It should have almost no impact on performance when not in use. The console command requires developer tools to be enabled.

Careful, the CSV files can get very large very fast :) One small-ish test game with (a beta of) Iron Horse being profiled made a CSV file of 8 MB with more than 460k events recorded across 7 in-game days.

planetmaker
planetmaker previously approved these changes Dec 22, 2019
Copy link
Contributor

@planetmaker planetmaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such feature will totally be helpful for more complex NewGRFs and their profiling :)

@andythenorth
Copy link
Contributor

andythenorth commented Dec 22, 2019

Just extra info...

CB IDs: https://github.com/OpenTTD/OpenTTD/blob/master/src/newgrf_callbacks.h#L20

Filesize: 30 days of Horse profiling (in a small game) gives 3m line csv. Some Excel versions, Apple Numbers etc won't open large files (the limit is sometimes 65k). Libre Office, some Excel versions might do more. I'm thinking that grf authors will find it better to write a python script to summarise the data, or maybe a jupyter notebook or similar.

@planetmaker
Copy link
Contributor

Yes, but the deficiencies of spreadsheet programmes should not limit us in our debug output :)

src/console_cmds.cpp Outdated Show resolved Hide resolved
src/date.cpp Outdated Show resolved Hide resolved
src/newgrf.cpp Show resolved Hide resolved
src/newgrf_profiling.cpp Outdated Show resolved Hide resolved
src/newgrf_profiling.h Outdated Show resolved Hide resolved
src/newgrf_profiling.cpp Outdated Show resolved Hide resolved
src/newgrf_canal.cpp Outdated Show resolved Hide resolved
src/newgrf_cargo.cpp Outdated Show resolved Hide resolved
src/newgrf_railtype.cpp Show resolved Hide resolved
src/newgrf_roadtype.cpp Show resolved Hide resolved
@frosch123
Copy link
Member

frosch123 commented Dec 22, 2019

How about "GetDebugID"?

src/newgrf_spritegroup.cpp Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
@nielsmh nielsmh added this to the 1.10.0 milestone Jan 12, 2020
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine, would like to get @frosch123's OK first though...

@frosch123
Copy link
Member

I experimented a bit with this last week.
I found the output quite hard to parse, due to the weird wrap-around behaviour of ticks.
For vehicles the "item id" was some random noise that I did not recognise.

@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 12, 2020

I think most NML GRFs will have automatically assigned item id's, but it really ought to be the value you can use to figure out which specific engine, industry tile, or whatever, that's causing the activity.
Perhaps the ticks should be taken as difference from the data collection start, so the first measurement is always at zero? It might even be possible to handle integer wraparound like that, in case someone actually captures more than 64k ticks worth of events.

@nielsmh nielsmh dismissed frosch123’s stale review January 22, 2020 10:53

Changes implemented

@nielsmh
Copy link
Contributor Author

nielsmh commented Jan 22, 2020

I'm unsure if it's okay to squash this to one commit (it's a large changeset), or if I should put in the effort to try make logical commits. I don't think there is much to separate out.

@nielsmh nielsmh merged commit c8779fb into OpenTTD:master Jan 26, 2020
PeterN added a commit to PeterN/OpenTTD that referenced this pull request Jan 26, 2020
PeterN added a commit to PeterN/OpenTTD that referenced this pull request Jan 26, 2020
PeterN added a commit that referenced this pull request Jan 26, 2020
douiwby pushed a commit to douiwby/OpenTTD that referenced this pull request Apr 16, 2020
Adds a console command newgrf_profile to collect some profiling data about NewGRF action 2 callbacks and produce a CSV file.
douiwby pushed a commit to douiwby/OpenTTD that referenced this pull request Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants