-
Notifications
You must be signed in to change notification settings - Fork 53
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
Improve build times by reducing included headers #299
Conversation
I had to resubmit the PR because it was accidentally done onto the wrong branch, which triggered all CIs to fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I just have a question about a non-header change.
This PR is exposing some compiler warnings on Windows coming from upstream. We had suppressed those on 206 with pragmas on |
Ahh I see. I'll have to compile on Windows and see those warnings. If they're trivial I may be able to fix upstream |
You should be able to see them on CI here: https://build.osrfoundation.org/job/ign_rendering-pr-win/1547/msbuild/
That would be awesome to get on the 2.2 version that we're about to build against, if possible 😃 |
+1 since ign-rendering4 (and ign-rendering5) depends on 2.1, we'll probably still need to add those pragma's in this PR around places where we include ogre headers. looks like this PR is branched out of |
53d8bc5
to
85dd1b9
Compare
|
For the 5 windows warnings, I think we can just give it the same treatment as we did in the ogre 1.x case: here's a diff
|
Ogre2Includes.hh is a very convenient header but it includes too many Ogre headers, which heavily affects build times. For certain .cc files compilation time was reduced by 1.2 seconds on an i7 7700 @3.60Ghz A full build was of ign-rendering5 went from 1m 57s to 1m 46s (I only touched ogre2, not ogre; a full rebuild of ign-rendering5 includes both). This amounts to an 11 second improvement for a full build meaning a 9.4% reduction (more if we consider ogre1 module is affecting measuring) The solution is to simply include Ogre headers that are needed, instead of relying on Ogre2Includes.hh; and make sure Ogre2Includes.hh is never included in a *.hh file. Signed-off-by: Matias N. Goldberg <[email protected]>
Updated again. The windows CI now passes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Ogre2Includes.hh is a very convenient header but it includes too many Ogre headers, which heavily affects build times. For certain .cc files compilation time was reduced by 1.2 seconds on an i7 7700 @3.60Ghz A full build was of ign-rendering5 went from 1m 57s to 1m 46s (I only touched ogre2, not ogre; a full rebuild of ign-rendering5 includes both). This amounts to an 11 second improvement for a full build meaning a 9.4% reduction (more if we consider ogre1 module is affecting measuring) The solution is to simply include Ogre headers that are needed, instead of relying on Ogre2Includes.hh; and make sure Ogre2Includes.hh is never included in a *.hh file. Signed-off-by: Matias N. Goldberg <[email protected]> Signed-off-by: Ian Chen <[email protected]>
Summary
Ogre2Includes.hh is a very convenient header but it includes too many
Ogre headers, which heavily affects build times.
For certain .cc files compilation time was reduced by 1.2 seconds on an
i7 7700 @3.60Ghz
A full build was of ign-rendering5 went from 1m 57s to 1m 46s (I only
touched ogre2, not ogre; a full rebuild of ign-rendering5 includes
both; optix was not included in my config).
This amounts to an 11 second improvement for a full build meaning a 9.4%
reduction (more if we consider ogre1 module is affecting measuring)
The solution is to simply include the Ogre headers that are needed, instead
of relying on Ogre2Includes.hh; and making sure Ogre2Includes.hh is never
included in a *.hh file.
Checklist
This is a simple header inclusion change; it does not alter runtime functionality.
Note that merging this change into other branches such as the WIP 2.2 might cause a few compiler errors as there are fewer headers included. But those build errors should be trivial fix.