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

Feature: NewGRF callback profiling #7868

merged 16 commits into from Jan 26, 2020

Conversation

@nielsmh
Copy link
Contributor

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.

Copy link
Contributor

planetmaker left a comment

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

@andythenorth

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

planetmaker commented Dec 22, 2019

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

This comment has been minimized.

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
@nielsmh nielsmh force-pushed the nielsmh:newgrf-perf branch from 00d391c to b5f6694 Dec 25, 2019
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 requested a review from frosch123 Dec 30, 2019
@nielsmh nielsmh added this to the 1.10.0 milestone Jan 12, 2020
Copy link
Member

LordAro left a comment

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

@frosch123

This comment has been minimized.

Copy link
Member

frosch123 commented Jan 12, 2020

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

This comment has been minimized.

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 Jan 22, 2020

Changes implemented

@nielsmh

This comment has been minimized.

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
8 checks passed
8 checks passed
OpenTTD CI Build #20191229.5 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.