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

Leverage forward declarations to improve compile times #990

Conversation

Levi-Armstrong
Copy link
Contributor

@Levi-Armstrong Levi-Armstrong commented Feb 22, 2024

This cuts the build time from 4 minutes and 30 seconds to 2 minutes and 45 seconds.

@marip8
Copy link
Contributor

marip8 commented Feb 22, 2024

Are the fwd.h headers not problematic by declaring a bunch of classes that don't necessarily get used in each source file? Why not just declare the specific classes that each header needs to reference?

@Levi-Armstrong
Copy link
Contributor Author

Are the fwd.h headers not problematic by declaring a bunch of classes that don't necessarily get used in each source file?

It is not because you can declare things as many time as you want but you can only have one define it once.

Why not just declare the specific classes that each header needs to reference?

Because forward declaration can be problematic especially when using in template classes, so it is encouraged that developers provide this file to let users know what types are okay to use as forward declarations. An example of this is the #include <iosfwd> provided by the standard libraries to avoid having IO headers in your headers.

@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch 2 times, most recently from 53e7d8f to 6f3d953 Compare March 8, 2024 15:38
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.17%. Comparing base (d361f7d) to head (fccd356).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #990      +/-   ##
==========================================
- Coverage   90.91%   90.17%   -0.74%     
==========================================
  Files         280      280              
  Lines       15876    15701     -175     
==========================================
- Hits        14434    14159     -275     
- Misses       1442     1542     +100     

see 181 files with indirect coverage changes

@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch 3 times, most recently from 372b9db to b6bad0d Compare March 9, 2024 03:17
@Levi-Armstrong
Copy link
Contributor Author

@johnwason Would you have time to look into what the issue is with the windows test failures?

@johnwason
Copy link
Contributor

Why not just use precompiled headers instead of reworking everything?

@Levi-Armstrong
Copy link
Contributor Author

Why not just use precompiled headers instead of reworking everything?

At this point I am almost finished updating everything.

@johnwason
Copy link
Contributor

@Levi-Armstrong my fixes didn't work... go ahead and force push to revert my last two commits.

@johnwason
Copy link
Contributor

I am not seeing the serialization unit test failures locally. I am seeing issues with the static loader plugin tests.

@Levi-Armstrong
Copy link
Contributor Author

@johnwason This may be related.

@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch 2 times, most recently from 64179cb to f9665d4 Compare March 11, 2024 03:23
@johnwason
Copy link
Contributor

I am not seeing the serialization unit tests locally. I am investigating but so far don't have an explanation. Maybe try clearing the github action cache completely?

@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch from 4c4d468 to 67e762d Compare March 11, 2024 17:36
@Levi-Armstrong
Copy link
Contributor Author

Can I manually clear the cache?

@johnwason
Copy link
Contributor

You can see all the caches here: https://github.com/tesseract-robotics/tesseract/actions/caches

Not sure how to "clear all" though

@johnwason
Copy link
Contributor

I updated to the newest vcpkg boost version and now I am seeing the test failures. I will keep trying to trace the error.

@johnwason
Copy link
Contributor

Looking at the CI, the Conda Windows CI is working with Boost 1.82, while the vcpkg Windows build is failing with Boost 1.84. This may be a problem with Boost 1.84.

@johnwason
Copy link
Contributor

Yeah I am seeing the same failures on the scheduled builds on master. This looks like a boost problem.

@johnwason
Copy link
Contributor

I ran a matrix build of different boost versions, and only 1.84 is failing. https://github.com/johnwason/tesseract/actions/runs/8242279639/job/22540993793

I set the vcpkg revision to use boost 1.83. I recommend opening an issue with boost serialization.

@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch 2 times, most recently from 57295f9 to b98277c Compare March 18, 2024 00:59
@Levi-Armstrong
Copy link
Contributor Author

@johnwason I believe I removed your change when I force pushed. What change do I need to make to pin the version?

@johnwason
Copy link
Contributor

@Levi-Armstrong I pushed the commit

@johnwason
Copy link
Contributor

See boost/serialization issue boostorg/serialization#309

@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch from 91f5d49 to c26eb15 Compare March 18, 2024 02:35
@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch from c26eb15 to 5c4cd2b Compare March 21, 2024 03:03
@Levi-Armstrong Levi-Armstrong force-pushed the feat/UseForwardDeclarations branch from 1bc9dc4 to fccd356 Compare March 25, 2024 12:03
@Levi-Armstrong Levi-Armstrong merged commit ea3d5c3 into tesseract-robotics:master Apr 10, 2024
12 of 13 checks passed
@Levi-Armstrong Levi-Armstrong deleted the feat/UseForwardDeclarations branch April 10, 2024 19:48
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.

3 participants