Add license field in Stackage class. #66

130s · 2016-12-31T09:17:01Z

Rosstackage class does probably not necessarily map exactly all the elements defined in package.xml, but from the api perspective, it'll be beneficial to be able to access more elements through this class so that client codes won't have to parse xml when needed (here this is related to #65).

Possible downside is that whenever rospack crawls it could take longer since more parsing happens (though I haven't tested), and for particularly usecases where license info is not needed this added info is simply a waste of time. I'm not sure how critical rospack's performance is, so I appreciate opinions.

dirk-thomas · 2016-12-31T22:50:15Z

Thank you for working on this. Since the performance of rospack is very critical it is important to quantify the impact of this patch on that aspect. Can you please run some measurements to compare the runtime with and without this patch on a larger number of packages (>100). Please make sure to compare the results without being impacted by the rospack and other factors (e.g. a different the build type).

130s · 2017-01-21T19:35:31Z

Ok, I made simple scripts (that I also pushed in 6f3fa0b for sharing purpose) and did small benchmark tests.

Following is the result of running rospack depends gmapping 50 times with and without the change in Rosstackage if this PR. In a nutshell, 0.5 second slower with the change.
I'm not sure if this latency is acceptable or not.

With the change:

$ git checkout jade-devel
Switched to branch 'jade-devel'
Your branch is up-to-date with 'origin/jade-devel'.
$ git checkout -b j/only_benchmark
Switched to a new branch 'j/only_benchmark'
$ git cherry-pick 6f3fa0b9b5d23b1a6f927848e0cd8f971ff9f2b2
[j/only_benchmark 2a2690f] Add simple benchamrk scripts.
 2 files changed, 67 insertions(+)
 create mode 100755 test/benchmark/iterate_command.sh
 create mode 100755 test/benchmark/measure_performace.sh
$ cd benchmark/
$ git log
commit 2a2690fa9963be5ef1347aa3b7d48edf1b3dd598
Author: Isaac I.Y. Saito <130s@2000.jukuin.keio.ac.jp>
Date:   Sat Jan 21 04:59:49 2017 -0800

    Add simple benchamrk scripts.

commit 205f5bc7da190e7c021956fff6092e932b191a6e
Author: Dirk Thomas <dthomas@osrfoundation.org>
Date:   Fri Sep 2 11:26:39 2016 -0700

    2.3.1
:
$ source ./measure_performace.sh
$ measure_performance_rospack "rospack depends gmapping" 50
Command to be measured: rospack depends gmapping
1th iteration.
cpp_common
rostime
:
tf
2th iteration.
cpp_common
rostime
:
tf
50th iteration.
cpp_common
rostime
:
tf
$ more rospack_50.log 
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 99.99    2.648000       25709       103        52 wait4
  0.01    0.000149           3        51           clone
  0.00    0.000000           0        10           read
  0.00    0.000000           0        50           write
  0.00    0.000000           0        39        30 open
  0.00    0.000000           0        11           close
  0.00    0.000000           0        30        16 stat
  0.00    0.000000           0         8           fstat
  0.00    0.000000           0         3           lseek
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        28           brk
  0.00    0.000000           0       111           rt_sigaction
  0.00    0.000000           0       415           rt_sigprocmask
  0.00    0.000000           0        51           rt_sigreturn
  0.00    0.000000           0         1         1 ioctl
  0.00    0.000000           0         9         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           dup2
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         3         1 fcntl
  0.00    0.000000           0         2           getrlimit
  0.00    0.000000           0         5           getuid
  0.00    0.000000           0         5           getgid
  0.00    0.000000           0         5           geteuid
  0.00    0.000000           0         5           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00    2.648149                   978       105 total

Without:

$ more rospack_50.log
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 99.99    2.188000       21451       102        51 wait4
  0.01    0.000117           0       415           rt_sigprocmask
  0.00    0.000000           0        10           read
  0.00    0.000000           0        50           write
  0.00    0.000000           0        39        30 open
  0.00    0.000000           0        11           close
  0.00    0.000000           0        30        16 stat
  0.00    0.000000           0         8           fstat
  0.00    0.000000           0         3           lseek
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        28           brk
  0.00    0.000000           0       111           rt_sigaction
  0.00    0.000000           0        51           rt_sigreturn
  0.00    0.000000           0         1         1 ioctl
  0.00    0.000000           0         9         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           dup2
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0        51           clone
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         3         1 fcntl
  0.00    0.000000           0         2           getrlimit
  0.00    0.000000           0         5           getuid
  0.00    0.000000           0         5           getgid
  0.00    0.000000           0         5           geteuid
  0.00    0.000000           0         5           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00    2.188117                   977       104 total

$ more /proc/cpuinfo 
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 60
model name      : Intel(R) Core(TM) i7-4710MQ CPU @ 2.50GHz
stepping        : 3
microcode       : 0x17
cpu MHz         : 3230.957
cache size      : 6144 KB
physical id     : 0
siblings        : 8
core id         : 0
cpu cores       : 4
:

130s · 2017-01-23T03:27:55Z

Testing more, actually I take back what I said in the previous post #66 (comment) -- there's no obvious latency using this PR. In some tests that use this PR actually are faster than without this PR. This time I ran the following command 500 times (all units in second):

Command	With PR#66	Without PR#66
rospack profile	8.30[1]	8.35[2]
rospack depends pr2_navigation_perception	24.99[3]	24.97[4]
rospack depends gmapping	24.61[5]	24.95[6]

Looking at the result I would say the time diff is negligible. I just don't understand why this PR doesn't slow it down. Some things I checked to verify that the test method is reasonable:

Stop as many processes as possible (to minimize the impact to rospack).
ROS_CACHE_TIMEOUT is set 0.0 so cache should be built per every rospack call if I understand correctly.
Remove catkin workspace and rebuild rospack every time switching branches.

You can download and run the benchmark tests:

mkdir -p cws_rospack_test/src && cd cws_rospack_test/src
git clone https://github.com/130s/rospack.git && cd rospack/
git checkout j/add/license_to_stackage 
cd ../..
catkin config --no-install && catkin b rospack
source devel/setup.bash
source `rospack find rospack`/test/benchmark/measure_performace.sh                                        

measure_performance "rospack profile" 500

Entire strace files.

[1]

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 99.99    8.304037        8287      1002       501 wait4
  0.01    0.000485           0      4016           rt_sigprocmask
  0.00    0.000270           0      1011           rt_sigaction
  0.00    0.000194           0       501           clone
  0.00    0.000000           0        20           read
  0.00    0.000000           0       500           write
  0.00    0.000000           0        38        30 open
  0.00    0.000000           0        10           close
  0.00    0.000000           0        58        37 stat
  0.00    0.000000           0         7           fstat
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        39           brk
  0.00    0.000000           0       501           rt_sigreturn
  0.00    0.000000           0        13         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1         1 getpeername
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         9           getuid
  0.00    0.000000           0         9           getgid
  0.00    0.000000           0         9           geteuid
  0.00    0.000000           0         9           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00    8.304986                  7786       574 total

[2]

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 99.99    8.352000        8335      1002       501 wait4
  0.01    0.000526           1       501           clone
  0.00    0.000337           0      1011           rt_sigaction
  0.00    0.000227           0      4016           rt_sigprocmask
  0.00    0.000054           0       500           write
  0.00    0.000000           0        20           read
  0.00    0.000000           0        38        30 open
  0.00    0.000000           0        10           close
  0.00    0.000000           0        59        38 stat
  0.00    0.000000           0         7           fstat
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        39           brk
  0.00    0.000000           0       501           rt_sigreturn
  0.00    0.000000           0        13         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1         1 getpeername
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         9           getuid
  0.00    0.000000           0         9           getgid
  0.00    0.000000           0         9           geteuid
  0.00    0.000000           0         9           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00    8.353144                  7787       575 total

[3]

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
100.00   24.992059       24942      1002       501 wait4
  0.00    0.000469           0      4016           rt_sigprocmask
  0.00    0.000213           0      1011           rt_sigaction
  0.00    0.000201           0       501           clone
  0.00    0.000113           0       501           rt_sigreturn
  0.00    0.000025           0       500           write
  0.00    0.000000           0        20           read
  0.00    0.000000           0        38        30 open
  0.00    0.000000           0        10           close
  0.00    0.000000           0        58        37 stat
  0.00    0.000000           0         7           fstat
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        39           brk
  0.00    0.000000           0        13         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1         1 getpeername
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         9           getuid
  0.00    0.000000           0         9           getgid
  0.00    0.000000           0         9           geteuid
  0.00    0.000000           0         9           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00   24.993080                  7786       574 total

[4]

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
100.00   24.972000       24922      1002       501 wait4
  0.00    0.000591           0      4016           rt_sigprocmask
  0.00    0.000302           1       501           clone
  0.00    0.000138           0       500           write
  0.00    0.000128           0       501           rt_sigreturn
  0.00    0.000075           0      1011           rt_sigaction
  0.00    0.000000           0        20           read
  0.00    0.000000           0        38        30 open
  0.00    0.000000           0        10           close
  0.00    0.000000           0        59        38 stat
  0.00    0.000000           0         7           fstat
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        39           brk
  0.00    0.000000           0        13         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1         1 getpeername
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         9           getuid
  0.00    0.000000           0         9           getgid
  0.00    0.000000           0         9           geteuid
  0.00    0.000000           0         9           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00   24.973234                  7787       575 total

[5]

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 99.99   24.612021       24563      1002       501 wait4
  0.00    0.001028           0      4016           rt_sigprocmask
  0.00    0.000327           0      1011           rt_sigaction
  0.00    0.000264           1       501           clone
  0.00    0.000155           0       501           rt_sigreturn
  0.00    0.000036           0       500           write
  0.00    0.000000           0        20           read
  0.00    0.000000           0        38        30 open
  0.00    0.000000           0        10           close
  0.00    0.000000           0        58        37 stat
  0.00    0.000000           0         7           fstat
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        39           brk
  0.00    0.000000           0        13         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1         1 getpeername
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         9           getuid
  0.00    0.000000           0         9           getgid
  0.00    0.000000           0         9           geteuid
  0.00    0.000000           0         9           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00   24.613831                  7786       574 total

[6]

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 99.99   24.952000       24902      1002       501 wait4
  0.00    0.000513           1      1011           rt_sigaction
  0.00    0.000420           0      4016           rt_sigprocmask
  0.00    0.000246           0       501           clone
  0.00    0.000113           0       501           rt_sigreturn
  0.00    0.000061           0       500           write
  0.00    0.000000           0        20           read
  0.00    0.000000           0        38        30 open
  0.00    0.000000           0        10           close
  0.00    0.000000           0        59        38 stat
  0.00    0.000000           0         7           fstat
  0.00    0.000000           0        15           mmap
  0.00    0.000000           0         8           mprotect
  0.00    0.000000           0         2           munmap
  0.00    0.000000           0        39           brk
  0.00    0.000000           0        13         5 access
  0.00    0.000000           0         1           pipe
  0.00    0.000000           0         1           getpid
  0.00    0.000000           0         1         1 getpeername
  0.00    0.000000           0         1           execve
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         9           getuid
  0.00    0.000000           0         9           getgid
  0.00    0.000000           0         9           geteuid
  0.00    0.000000           0         9           getegid
  0.00    0.000000           0         1           getppid
  0.00    0.000000           0         1           getpgrp
  0.00    0.000000           0         1           arch_prctl
------ ----------- ----------- --------- --------- ----------------
100.00   24.953353                  7787       575 total

dirk-thomas · 2017-01-25T18:35:23Z

include/rospack/rospack.h

@@ -146,6 +146,7 @@ class ROSPACK_DECL Rosstackage
 bool crawled_;
 std::string name_;
 std::string tag_;
+ std::vector<std::string> licenses_;


Since the new member is private it is not accessible by any external code. What is the purpose here?

Also this change will break ABI which needs to be considered to decide into which branch to merge this.

Removed this line. I can't remember, but I think this was added simply by mistake.

dirk-thomas · 2017-01-25T18:37:13Z

test/benchmark/measure_performace.sh

@@ -0,0 +1,47 @@
+#!/bin/bash


The script itself can't be run. Therefore it should neither be executable nor have a shebang line. It should probably output a message how the functions can be used when it is being sourced.

shebang removed.

The benchmark folder and its content is not specific to rospack at all so I can remove the commit that added it if that's better.

mgruhler · 2017-01-31T20:25:14Z

@130s nice. I was actually thinking of implementing something like this as well. Anything I could help with?

Edit:
Whoops, this was supposed to go into #56 I got lost between the PRs ;-)

).

dirk-thomas

LGTM now.

…).

130s mentioned this pull request Dec 31, 2016

[In progress] Add license feature #56

Closed

dirk-thomas added the more-information-needed label Jan 3, 2017

130s force-pushed the j/add/license_to_stackage branch 2 times, most recently from a885f10 to e52d453 Compare January 23, 2017 03:20

dirk-thomas requested changes Jan 25, 2017

View reviewed changes

Add license field in Stackage class.

d513117

130s force-pushed the j/add/license_to_stackage branch from e52d453 to 755f007 Compare February 8, 2017 10:20

130s added a commit to kinu-garage/hut_10sqft that referenced this pull request Feb 8, 2017

[util] Add performance measure script (orignally made for ros/rospack#66

4fc6ed8

).

130s mentioned this pull request Feb 8, 2017

[util] Add performance measure script kinu-garage/hut_10sqft#113

Merged

130s added a commit to kinu-garage/hut_10sqft that referenced this pull request Feb 8, 2017

[util] Add performance measure script (orignally made for ros/rospack#66

419479b

).

Add simple benchamrk script.

bc6d495

130s force-pushed the j/add/license_to_stackage branch from 755f007 to bc6d495 Compare February 8, 2017 11:23

dirk-thomas removed the more-information-needed label Feb 8, 2017

dirk-thomas approved these changes Feb 8, 2017

View reviewed changes

dirk-thomas merged commit f2a82d6 into ros:jade-devel Feb 8, 2017

130s deleted the j/add/license_to_stackage branch February 8, 2017 18:12

130s changed the title ~~Add license field in Rosstackage class.~~ Add license field in Stackage class. Feb 8, 2017

130s mentioned this pull request Feb 25, 2017

[Indigo] Add license field in Stackage class. (cherrypick #66) #69

Closed

dirk-thomas mentioned this pull request Mar 1, 2017

changes between 2.2.7 and 2.3.3 for backporting #72

Merged

130s added a commit to 130s/rospack that referenced this pull request May 5, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

cc90d7c

…).

130s added a commit to 130s/rospack that referenced this pull request Jun 11, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

88b9515

…).

130s added a commit to 130s/rospack that referenced this pull request Jun 11, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

6c78eb3

…).

130s added a commit to 130s/rospack that referenced this pull request Jun 11, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

b4eaf2a

…).

130s added a commit to 130s/rospack that referenced this pull request Jun 11, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

9eec8b1

…).

130s added a commit to 130s/rospack that referenced this pull request Jun 11, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

6809ceb

…).

130s added a commit to 130s/rospack that referenced this pull request Oct 16, 2017

[license feature] Use Stackage.license_ field (that was added in ros#66…

de543e4

…).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add license field in Stackage class. #66

Add license field in Stackage class. #66

130s commented Dec 31, 2016 •

edited

Loading

dirk-thomas commented Dec 31, 2016

130s commented Jan 21, 2017

130s commented Jan 23, 2017 •

edited

Loading

dirk-thomas Jan 25, 2017

130s Feb 8, 2017

dirk-thomas Jan 25, 2017

130s Feb 8, 2017

mgruhler commented Jan 31, 2017 •

edited

Loading

dirk-thomas left a comment

Add license field in Stackage class. #66

Add license field in Stackage class. #66

Conversation

130s commented Dec 31, 2016 • edited Loading

dirk-thomas commented Dec 31, 2016

130s commented Jan 21, 2017

130s commented Jan 23, 2017 • edited Loading

dirk-thomas Jan 25, 2017

Choose a reason for hiding this comment

130s Feb 8, 2017

Choose a reason for hiding this comment

dirk-thomas Jan 25, 2017

Choose a reason for hiding this comment

130s Feb 8, 2017

Choose a reason for hiding this comment

mgruhler commented Jan 31, 2017 • edited Loading

dirk-thomas left a comment

Choose a reason for hiding this comment

130s commented Dec 31, 2016 •

edited

Loading

130s commented Jan 23, 2017 •

edited

Loading

mgruhler commented Jan 31, 2017 •

edited

Loading