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

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

Merged
merged 1 commit into from Jun 10, 2015

Conversation

doudou
Copy link
Contributor

@doudou doudou commented Oct 7, 2014

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
Copy link
Contributor

marvin2k commented Oct 7, 2014

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
Copy link
Contributor Author

doudou commented Oct 7, 2014

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

@doudou
Copy link
Contributor Author

doudou commented Oct 7, 2014

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
Copy link
Contributor

marvin2k commented Oct 9, 2014

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
Copy link
Contributor Author

doudou commented Oct 13, 2014

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
Copy link
Contributor Author

doudou commented Oct 13, 2014

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
Copy link
Contributor Author

doudou commented Oct 13, 2014

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
Copy link
Contributor

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
Copy link
Contributor Author

doudou commented Oct 14, 2014

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.

@doudou doudou changed the title Streamline headers included in task base DO NOT MERGE: Streamline headers included in task base Oct 14, 2014
@marvin2k
Copy link
Contributor

marvin2k commented Jan 8, 2015

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):

  • git.hb.dfki.de/sherpa/orogen-sherpa_manipulator_kinematics -- had to change existing Task.hpp to make it compile. The header in question (base/commands/Joints.hpp) is used in an output-port of the orogen-file. It should be in the projects typekit and hence been added to TaskBase.hpp? It is not, but /uin32_t is added as output instead and I don't get where this comes from... Same for gccxml- and clang-based importers.
  • gitorious.org/rock-control/orogen-skid4_control.git -- similar. Uses base/backward/base/actuators/vehicles.h which is deprecated since 1.5years without replacement?
  • gitorious.org/rock-slam/orogen-tilt_scan -- similar

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

@doudou
Copy link
Contributor Author

doudou commented Jan 14, 2015

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

2maz added a commit to rock-multiagent/multiagent-orogen-fipa_services that referenced this pull request May 28, 2015
@goldhoorn
Copy link
Member

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?)

dmronga added a commit to rock-control/control-orogen-cart_ctrl_wdls that referenced this pull request May 29, 2015
dmronga added a commit to rock-control/control-orogen-trajectory_generation that referenced this pull request May 29, 2015
saarnold added a commit to rock-slam/slam-orogen-pose_estimation that referenced this pull request May 29, 2015
saarnold added a commit to rock-control/control-orogen-auv_rel_pos_controller that referenced this pull request May 29, 2015
jmachowinski added a commit to rock-control/control-orogen-waypoint_navigation that referenced this pull request May 29, 2015
goldhoorn added a commit to rock-drivers/drivers-orogen-controldev that referenced this pull request May 29, 2015
jmachowinski added a commit to rock-slam/slam-orogen-tilt_scan that referenced this pull request May 29, 2015
ajishbabu added a commit to rock-control/control-orogen-skid4_control that referenced this pull request May 29, 2015
doudou added a commit to rock-control/control-orogen-motor_controller that referenced this pull request May 29, 2015
…Base.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
@goldhoorn
Copy link
Member

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

jhidalgocarrio added a commit to rock-perception/perception-orogen-viso2 that referenced this pull request Jun 2, 2015
goldhoorn added a commit to rock-drivers/drivers-orogen-servo_dynamixel that referenced this pull request Jun 3, 2015
@doudou
Copy link
Contributor Author

doudou commented Jun 3, 2015

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

@goldhoorn goldhoorn force-pushed the streamline_headers_included_in_TaskBase branch from c956ee2 to 2f2ba49 Compare June 4, 2015 09:00
@goldhoorn
Copy link
Member

done

@goldhoorn goldhoorn changed the title DO NOT MERGE: Streamline headers included in task base merge date 10.06.2015: Streamline headers included in task base Jun 8, 2015
goldhoorn added a commit that referenced this pull request Jun 10, 2015
…ded_in_TaskBase

merge date 10.06.2015: Streamline headers included in task base
@goldhoorn goldhoorn merged commit cebd198 into master Jun 10, 2015
@goldhoorn goldhoorn deleted the streamline_headers_included_in_TaskBase branch June 10, 2015 05:05
dmronga added a commit to ARC-OPT/orogen-wbc that referenced this pull request May 19, 2022
dmronga added a commit to ARC-OPT/orogen-ctrl_lib that referenced this pull request May 19, 2022
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.

None yet

3 participants