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

split-off input_context.h from input.h; run iwyu to always include input.h directly #71141

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

Brambor
Copy link
Contributor

@Brambor Brambor commented Jan 21, 2024

Summary

None

Purpose of change

Split-off input_context.h from src/input.h to reduce compilation time when changing something in header files.
Test how well did I do.
I want to split src/input.h in another PR.

I realize that, since there are unneeded includes, it still can happen, that I recompile more than I need to (when working with input.h). But the ratio (of files needing recompilation after the future input.h split vs before) will say what the ideal (lowest) recompilation file count would be if all indirect includes were sorted out.

Sorting out these unneeded includes would take too much time. I do only small bites.

Describe the solution

run iwyu

  • First commit: accept (not all) changes that only remove includes / forward declarations (don't add any). I might as well do this since I run iwyu anyway. (almost pure, had to add lightmap.h include)
  • Second commit: includes in input.h, input.cpp according to iwyu.
  • Third commit: expand includes so that #include "input.h" is directly exposed everywhere. On affected files, remove whatever iwyu reports to remove, but add only the minimum needed for compilation (take advantage of indirect includes).
  • check iwyu (tests below)
  • Fourth commit: split-off input_context.h and input_enums.h from input.h and input_context.cpp from input.cpp
    • input_enums.h contains things that both of the other header files need. It is included in both of the header files.
  • Fifth commit: like third, but expand input.h and input_context.h, if neither is needed, but input_enums.h, include it.
  • Test how effective this is by counting the number of files needing recompilation before and after this change.

Conclusion

Test results bellow.

I set out to reduce re-compilation times for class input_manager in src/input.h. It is now (indirectly) included in 36 rather than 240 files! So this was a resounding success.

I believe, however, that the success comes from splitting the input.h file, not from running iwyu alone. As can be seen from not reducing includes for split-off input_context.h. Therefore I don't think running iwyu on the whole codebase is worth my time right now. However, it seems that splitting monolithic header files would have a significant impact.

Describe alternatives you've considered

Running over the whole codebase. But I want something merge-able. And small bites.

Testing

compiling locally
compiling in this pr

(running) iwyu

  • suggests no changes for input.h and input.cpp
  • suggests no changes for input_context.h and input_context.cpp
  • doesn't suggest to add or remove input.h or input_context.h anywhere
  • suggests to add input_enums.h only if input.h or input_context.h are already included

Additional context

Not a draft, so that all tests run. Ready to merge now!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Auto-requesting reviews from non-collaborators: @andrei8l

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Info / User Interface Game - player communication, menus, etc. Missions Quests and missions Map / Mapgen Overmap, Mapgen, Map extras, Map display Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` Monsters Monsters both friendly and unfriendly. json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Jan 21, 2024
@github-actions github-actions bot added Vehicles Vehicles, parts, mechanics & interactions Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels Jan 21, 2024
@Brambor Brambor changed the title run iwyu to split "input.h" (DON'T MERGE) run iwyu to always include "input.h" directly (DON'T MERGE) Jan 21, 2024
@Brambor Brambor changed the title run iwyu to always include "input.h" directly (DON'T MERGE) run iwyu to always include "input.h" directly Jan 21, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Jan 21, 2024

If the tests pass, this is ready to be merged!

@akrieger
Copy link
Member

Clang tidy says

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/lightmap.h:45:36: error: no member named 'log' in namespace 'std' [clang-diagnostic-error]
    return static_cast<int>( -std::log( LIGHT_AMBIENT_LOW / b ) * ( 1.0 /

@Brambor Brambor changed the title run iwyu to always include "input.h" directly split-off input_context.h from input.h; run iwyu to always include input.h directly Jan 22, 2024
@Brambor Brambor changed the title split-off input_context.h from input.h; run iwyu to always include input.h directly [WIP] split-off input_context.h from input.h; run iwyu to always include input.h directly Jan 22, 2024
@github-actions github-actions bot added Bionics CBM (Compact Bionic Modules) Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies Character / World Generation Issues and enhancements concerning stages of creating a character or a world Player Faction Base / Camp All about the player faction base/camp/site Items: Containers Things that hold other things Game: Achievements / Conducts / Scores Player goals and how they are tracked. Martial Arts Arts, Techniques, weapons and anything touching martial arts. Items: Armor / Clothing Armor and clothing labels Jan 22, 2024
@Brambor Brambor changed the title [WIP] split-off input_context.h from input.h; run iwyu to always include input.h directly split-off input_context.h from input.h; run iwyu to always include input.h directly Jan 22, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Jan 22, 2024

Clang tidy says

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/lightmap.h:45:36: error: no member named 'log' in namespace 'std' [clang-diagnostic-error]
    return static_cast<int>( -std::log( LIGHT_AMBIENT_LOW / b ) * ( 1.0 /

It should be resolved now.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 23, 2024
@ehughsbaird
Copy link
Contributor

Test how effective this is by counting the number of files needing recompilation before and after this change.

Before

% cat obj/*.inc | grep input.h | wc -l
243

After

% cat obj/*.inc | grep input_context.h | wc -l
240
% cat obj/*.inc | grep input_enums.h | wc -l
244
% cat obj/*.inc | grep input.h | wc -l
36

@Brambor
Copy link
Contributor Author

Brambor commented Jan 23, 2024

I wondered if the includes in that file could repeat, so I did this, and they don't seem to be repeating.

% grep -m 1 input_context.h obj/*.inc | wc -l
240

split-off "input_context.h" and "input_enums.h" from "input.h" and "input_context.cpp" from "input.cpp"
"input_enums.h" contains things that both of the other header files need.
Therefor it is included in both of the header files.
…xcessive includes

for all files that iwyu reports as (it should add or remove "input.h" or "input_context.h")
{
        add / remove "input.h" and "input_context.h" according to iwyu;
        remove everything iwyu reports to remove;
        while (doesn't compile) {add directly what needs to be added};
        /* note: thanks to indirect includes, we don't need to include most of what iwyu reports
        we obey iwyu only for "input.h" and "input_context.h", everything else is a suggestion
        if it doesn't compile without it */
}
@Brambor
Copy link
Contributor Author

Brambor commented Jan 24, 2024

Nooooooooooooooooo
A merge conflict :(
Why do you do this to me?

Looks so simple.
But I will have to rerun this.
Will take about an hour....

@Maleclypse
Copy link
Member

Sorry! Ping me in discord when it's passed tests after merge conflict resolution.

@Brambor
Copy link
Contributor Author

Brambor commented Jan 24, 2024

Let's just try including both, what could go wrong?

@Brambor Brambor closed this Jan 24, 2024
@Brambor Brambor reopened this Jan 24, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Jan 24, 2024

test failed, restarting

@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 24, 2024
@Brambor Brambor closed this Jan 24, 2024
@Brambor Brambor reopened this Jan 24, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Jan 24, 2024

trying to restart again

Clang 12 Asan cancelled during compilation for some reason, so all tests after that also didn't run. Someone should give this a kick probably.

@akrieger
Copy link
Member

widget_test segfaulted on both windows and mac, which is slightly suspicious. If it were just one, I wouldn't care, but those are two wildly different platforms.

@andrei8l
Copy link
Contributor

widget_test segfaulted on both windows and mac, which is slightly suspicious. If it were just one, I wouldn't care, but those are two wildly different platforms.

That's my fault. See #71213

@Maleclypse Maleclypse merged commit dedc16b into CleverRaven:master Jan 25, 2024
54 of 75 checks passed
@Brambor Brambor deleted the iwyu branch January 25, 2024 13:06
@Brambor
Copy link
Contributor Author

Brambor commented Jan 25, 2024

As promised, I now need to recompile only 38 files, which is over 6x improvement. I love it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Appliance/Power Grid Anything to do with appliances and power grid astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Bionics CBM (Compact Bionic Modules) [C++] Changes (can be) made in C++. Previously named `Code` Character / World Generation Issues and enhancements concerning stages of creating a character or a world Code: Tests Measurement, self-control, statistics, balancing. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Game: Achievements / Conducts / Scores Player goals and how they are tracked. Info / User Interface Game - player communication, menus, etc. Items: Armor / Clothing Armor and clothing Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display Martial Arts Arts, Techniques, weapons and anything touching martial arts. Missions Quests and missions Monsters Monsters both friendly and unfriendly. Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies NPC / Factions NPCs, AI, Speech, Factions, Ownership Player Faction Base / Camp All about the player faction base/camp/site Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants