[roslaunch] cache result of path finding for faster roslaunch behavior by wkentaro · Pull Request #676 · ros/ros_comm

wkentaro · 2015-09-29T16:10:45Z

This is for faster roslaunch's node launching.

Closes #541
Closes #542

This is firstly reported by @garaemon .

ros-pull-request-builder · 2015-09-29T16:10:47Z

Can one of the admins verify this patch?

wkentaro · 2015-09-29T16:57:56Z

Demo: https://drive.google.com/uc?id=0B9P1L--7Wd2veFZkSl82NzI4YVE

dirk-thomas · 2015-10-07T17:41:17Z

tools/roslaunch/src/roslaunch/rlutil.py

pkg and resource_name are not keyword arguments. Therefore please pass them as positional arguments.

I think they are keyword arguments.
https://github.com/ros/ros/blob/jade-devel/core/roslib/src/roslib/packages.py#L463

and cached.py in this package supports both.
https://github.com/ros/ros_comm/pull/676/files#diff-66abf4062401099443c44fc29a06e40aR28

cached.find_resource(args[0], args[1])

cached.find_resource(pkg=args[0], resource_name=args[1])

The change I made about the arguments are just a refactoring.
Using keyword arguments is good habit I think.

You are passing the two standard arguments as if they were keyword arguments. Please pass them without the keyword instead.

wkentaro · 2015-10-08T09:59:10Z

I updated the commit, so please review again.

dirk-thomas · 2015-10-08T19:15:37Z

tools/roslaunch/src/roslaunch/substitution_args.py

Please avoid unrelated white space changes.

dirk-thomas · 2015-10-08T19:27:59Z

Beside the comment the PRs look good. the performance improvement is definitely notable when the launch file has several resources to look up.

wkentaro · 2015-10-08T20:18:18Z

I updated the commit following your comments.
Please review again.

dirk-thomas · 2015-10-08T20:19:35Z

tools/roslaunch/src/roslaunch/node_args.py

Please move the import one line up to follow PEP8.

wkentaro · 2015-10-08T20:22:25Z

I updated again.

dirk-thomas · 2015-10-08T20:30:51Z

tools/roslaunch/src/roslaunch/resource_cache.py

Sorry for the late tiny feedback but some of it only appears when I open the code in my editor which does all kind of style checks.

Please change first word from Returns to Return (PEP257 D401).
Please add an empty line between this and the next line (PEP257 D205).

wkentaro · 2015-10-09T06:08:39Z

I updated the commit. Please review again.

dirk-thomas · 2015-10-09T16:32:20Z

Thanks for iterating on the patch.

[roslaunch] cache result of path finding for faster roslaunch behavior

wkentaro · 2015-10-09T16:44:17Z

Thanks! 😄

garaemon · 2015-10-10T04:46:09Z

awesome feature!! :)

dirk-thomas · 2015-10-12T19:23:22Z

I reviewed the change again and think an alternative approach will be even better. Currently the already passed in rospack instance is supposed to do some caching. But for separate nodes new instances are created. The following patches revert the previous changes and ensure that a single rospack instance is used which also caches the additional package mapping information:

@wkentaro Can you please confirm that these changes also work in your scenario and that they improve performance even more?

dirk-thomas · 2015-10-13T00:49:03Z

I have reverted the changes from this PR in favor of #682.

[roslaunch] cache result of path finding for faster roslaunch behavior

This was referenced Sep 29, 2015

[roslib] Args for cache the result of path finding in catkin.find_in_workspaces ros/ros#91

Closed

[python] Collect path data which can be resused for find_in_workspaces ros/catkin#757

Closed

wkentaro changed the title ~~[roslaunch] cache result of path finding~~ [roslaunch] cache result of path finding for faster roslaunch behavior Sep 29, 2015

wkentaro changed the title ~~[roslaunch] cache result of path finding for faster roslaunch behavior~~ [indigo] [roslaunch] cache result of path finding for faster roslaunch behavior Sep 29, 2015

wkentaro changed the title ~~[indigo] [roslaunch] cache result of path finding for faster roslaunch behavior~~ [roslaunch] cache result of path finding for faster roslaunch behavior Sep 29, 2015

This was referenced Sep 29, 2015

[roslaunch] cache result of catkin_pkg.packages.find_packages to speed up roslaunch #541

Closed

Enable caching of catkin_pkg.packages.find_packages when using roslaunch #542

Closed

wkentaro force-pushed the cache-result-of-find-packages branch 3 times, most recently from 2424fcc to 09d9fa1 Compare September 29, 2015 19:19

dirk-thomas reviewed Oct 7, 2015
View reviewed changes

wkentaro force-pushed the cache-result-of-find-packages branch from 09d9fa1 to 10e7bc2 Compare October 8, 2015 09:53

wkentaro force-pushed the cache-result-of-find-packages branch 3 times, most recently from 555882e to 892245a Compare October 8, 2015 11:13

dirk-thomas reviewed Oct 8, 2015
View reviewed changes

tools/roslaunch/src/roslaunch/substitution_args.py Outdated

Copy link
Copy Markdown

Member

dirk-thomas Oct 8, 2015

Choose a reason for hiding this comment

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

Please avoid unrelated white space changes.

wkentaro force-pushed the cache-result-of-find-packages branch 3 times, most recently from aaa7975 to 4fc55e9 Compare October 8, 2015 20:16

dirk-thomas reviewed Oct 8, 2015
View reviewed changes

tools/roslaunch/src/roslaunch/node_args.py Outdated

Copy link
Copy Markdown

Member

dirk-thomas Oct 8, 2015

Choose a reason for hiding this comment

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

Please move the import one line up to follow PEP8.

wkentaro force-pushed the cache-result-of-find-packages branch from 4fc55e9 to 293bf4c Compare October 8, 2015 20:22

dirk-thomas reviewed Oct 8, 2015
View reviewed changes

Cache result of path findings

72a9a91

wkentaro force-pushed the cache-result-of-find-packages branch from 293bf4c to 72a9a91 Compare October 9, 2015 06:07

dirk-thomas added a commit that referenced this pull request Oct 9, 2015

Merge pull request #676 from wkentaro/cache-result-of-find-packages

ea9a768

[roslaunch] cache result of path finding for faster roslaunch behavior

dirk-thomas merged commit ea9a768 into ros:indigo-devel Oct 9, 2015

wkentaro deleted the cache-result-of-find-packages branch October 9, 2015 16:39

wkentaro mentioned this pull request Oct 9, 2015

roslaunch is slow jsk-ros-pkg/jsk_common#617

Closed

k-okada mentioned this pull request Oct 11, 2015

Releasing new version #680

Closed

dirk-thomas mentioned this pull request Oct 12, 2015

Roslaunch performance improvement #682

Merged

garaemon mentioned this pull request Oct 15, 2015

'RosPack' object has no attribute 'get_custom_cache' jsk-ros-pkg/jsk_travis#170

Closed

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017

Merge pull request ros#676 from wkentaro/cache-result-of-find-packages

b04acb6

[roslaunch] cache result of path finding for faster roslaunch behavior

Conversation

wkentaro commented Sep 29, 2015

Uh oh!

ros-pull-request-builder commented Sep 29, 2015

Uh oh!

wkentaro commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkentaro commented Oct 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Oct 8, 2015

Uh oh!

wkentaro commented Oct 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkentaro commented Oct 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkentaro commented Oct 9, 2015

Uh oh!

dirk-thomas commented Oct 9, 2015

Uh oh!

wkentaro commented Oct 9, 2015

Uh oh!

garaemon commented Oct 10, 2015

Uh oh!

dirk-thomas commented Oct 12, 2015

Uh oh!

dirk-thomas commented Oct 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants