Skip to content

Conversation

@tridge
Copy link
Contributor

@tridge tridge commented Jun 20, 2018

This improves build speed for the common case of:
rm -rf build; ./waf configure --board fmuv3; ./waf plane

this PR changes us from the builtin c_preproc depenency checker in waf to the gcc based -MD system (using .d files). That reduces the time for the above build with ccache from 90 seconds to 9 seconds

It is also faster with no ccache. On my machine rover build drops from 3:25 to 2:47

Copy link
Contributor

Choose a reason for hiding this comment

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

This does increase the build size for minimized boards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I don't think it does (as its a union)

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked, it does change the build size here, and this is not a union, it's a struct. (Each driver type needs it's own detect state))

Copy link
Contributor

Choose a reason for hiding this comment

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

With the HAL_MINIMIZE_FEATURES check:

BUILD SUMMARY
Build directory: /home/wickedshell/code/ardupilot/build/fmuv2
Target         Text    Data  BSS     Total  
--------------------------------------------
bin/arduplane  952124  3960  193284  1149368

Without the HAL_MINIMIZE_FEATURES check:

BUILD SUMMARY
Build directory: /home/wickedshell/code/ardupilot/build/fmuv2
Target         Text    Data  BSS     Total  
--------------------------------------------
bin/arduplane  952172  3960  193284  1149416

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry about that, of course its a struct
now, the Q is if 50 bytes is worth it ...

Copy link
Contributor

Choose a reason for hiding this comment

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

The other option is to just move the detect state into AP_GPS.cpp as a static struct (or static member of AP_GPS::detect_instance()) which would allow the maintenance of the HAL_MINIMIZE_FEATURES check, without polluting the header.

Copy link
Member

Choose a reason for hiding this comment

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

Make builds already don't fit for PX4-FMUv2, so we're probably approaching it too with waf builds.

@tridge tridge force-pushed the pr-ccache-optimisation branch from 930bd0e to 555f302 Compare June 20, 2018 10:49
tridge added 4 commits June 21, 2018 14:42
making this header equal improves speed of fmuv2 build after fmuv3
build with ccache, without affecting binary
this is considerably faster than the way dependency system
@tridge tridge force-pushed the pr-ccache-optimisation branch from 555f302 to b80a869 Compare June 21, 2018 04:42
@OXINARF
Copy link
Member

OXINARF commented Jun 21, 2018

LGTM, except the change to waf: doesn't that mean that some dependencies won't be accounted for?

@tridge
Copy link
Contributor Author

tridge commented Jun 22, 2018

@OXINARF yes, it basically means the PX4 ORB generated headers are considered like system headers, and aren't considered for dependencies. Given those ORB msgs haven't changed for many months I don't think its an issue

@tridge
Copy link
Contributor Author

tridge commented Jun 22, 2018

merged after discussion with @WickedShell and @peterbarker

@tridge tridge merged commit c31e59e into ArduPilot:master Jun 22, 2018
@tridge
Copy link
Contributor Author

tridge commented Jun 22, 2018

@OXINARF I've added a DevCallTopic tag for further discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants