search

orocos-toolchain / orogen

Code generator for components and type handling in Rock - the Robot Construction Kit - and the Orocos Toolchain
http://rock-robotics.org
Other
4 stars 35 forks source link

merge date 10.06.2015: Streamline headers included in task base #15

Closed doudou closed 9 years ago

doudou commented 10 years ago

DO NOT MERGE: this will break the compilation of existing components (missing headers) so we'll have to give enough advance warning on the ML. Plus, there seem to currently have some issues in the generated code.

The main change is to remove the inclusion of the all-encompassing Types.hpp header in TaskBase.hpp

This saves both CPU time and memory during the build. On control/orogen/auv_control, which only imports the 'base' typekit, we save on my (very old Core 2 Duo) machine 1min 20s and 250M of memory (benchmark done with -j1). On -j4, this would mean roughly 1G of memory.

In addition, it avoids unnecessarily rebuilding task files. The Types.hpp headers include all types of a typekit, as well as the imported typekits, which means that we currently MUST rebuild all task files as soon as a single header is modified in e.g. base/types.

BACKWARD INCOMPATIBILITY: since a lot less headers are included, the user-facing code in Task.cpp / Task.hpp cannot assume that all types are available anymore. This does include typedefs that are used in the orogen file. All types must be manually included in Task.hpp. For instance, as base::commands::Joints is a typedef, one would need to include base/commands/Joints.hpp explicitly even if there is a port of type base::commands::Joints on the task. I personally think that it is worth it.

RAW DATA

Command being timed: "make -C build/tasks"

Without this commit: User time (seconds): 249.27 System time (seconds): 16.02 Percent of CPU this job got: 99% Elapsed (wall clock) time (h:mm:ss or m:ss): 4:27.81 Maximum resident set size (kbytes): 893588

With the patch: User time (seconds): 176.46 System time (seconds): 12.16 Percent of CPU this job got: 99% Elapsed (wall clock) time (h:mm:ss or m:ss): 3:09.72 Maximum resident set size (kbytes): 640356

marvin2k commented 10 years ago

The benchmark command:

/usr/bin/time -f "User time (seconds): %U\nSystem time (seconds): %S\nPercent of CPU this job got: %P\nElapsed (wall clock) time (h:mm:ss or m:ss): %E\nMaximum resident set size (kbytes): %t"
doudou commented 10 years ago

The benchmark command was /usr/bin/time -v. I've removed irrelevant lines

doudou commented 10 years ago

Lot of low-hanging fruits around?

While it is better, it is also very non-critical. I have other things to do with my life than replace all 'component' by 'project', so I rather do it step by step.

marvin2k commented 10 years ago

While it is better, it is also very non-critical

Then I would argue to not rename component at all. As it is very non-critical the commit just decreases the readability of the code-base. Someone entering the project cannot know what is "deprecated" and what not. One just assumes different functionality.

doudou commented 10 years ago

Probably a sane advice, but that's too late. It is probably 50/50 now, so upgrading bit by bit to the new one does seem like a not-so-bad course of action.

doudou commented 10 years ago

In addition: if readability / understandability is a concern, then 'component' is a really really bad name. This is actually the main reason why I started moving to 'project'.

I do agree that it's far from being ideal, but unfortunately (as you noticed yourself), orogen is a codebase that is far from being ideal. Given how both critical this package is, and low on resource/time we are, I am trying to improve it bit by bit. As an example, orogen_loaders seems big but is actually my fourth attempt at getting the smallest step that would allow to go towards the right direction.

doudou commented 10 years ago

Now that you have a bit more context, let's stop wasting our collective time. Either at this point you are OK with me merging as-is or I'll just remove the renaming to 'project'.

marvin2k commented 10 years ago

Renaming with the big hammer (sed) is not an option?

Sadly I did not find the time to actually profoundly test this -- sorry. I did one re-build, which failed. Did not investigate yet what went wrong, but it was somewhere inside the generated Code.

doudou commented 10 years ago

OK ... Will wait. As I mentioned on the ML I have only a 5 year old laptop, not really suitable to make full rock builds to test. Definitely should not be merged until we did :P Too eager to get this through I guess. The build will fail though, as some orogen components will have to be updated. Meaning, I should wait anyways and give some advance warning to the Rock devels / users.

marvin2k commented 9 years ago

This gets rather messy? Cannot update this PR. Rebased to current master and added two commits here -- one of them sugar, the other one needed. Then the basic compilation went through, but...

Second-party projects which needed changes (missing headers):

After fixing these three manually (didn't upload the trivial changes anywhere) the rest compiles. As usual: No runtime-checks.

doudou commented 9 years ago

As said on PR description, this will break some components.

The issue with base/commands/Joints.hpp is that base/commands/Joints is a simple typedef to base/samples/Joints, which means that typelib/orogen "loses" the type information (i.e. orogen believes that the type is base/samples/Joints). I don't understand the thing about uint32_t

goldhoorn commented 9 years ago

I created for all known rock packages a PR (which was really straight forward) I would propose to announce this on the ML and do the merge within the next weeks... (@doudou can you merge against master?)

goldhoorn commented 9 years ago

I done the rebase but i don't want to force-push on your PRs find the current patch here

doudou commented 9 years ago

@goldhoorn: please force-push it on the PR, I'm fine with that.

goldhoorn commented 9 years ago

done