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

cache lookup of importlib metadata in Node action #365

Merged
merged 2 commits into from
Jun 16, 2023

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 14, 2023

The short version of this is that we use importlib_metadata to load extensions to the Node action via Python's entry_point pattern. This lets new packages extend Node without changing launch_ros itself. The problem is that we load these extensions every time a node needs them and that can take as long as 1 second on some machines. If you have a launch file with hundreds of nodes, like I did, then you get pauses of 30 seconds to a minute before the launch file starts launching things.


I used the profiler to find out that this launch_ros.actions.Node.get_extensions() was being called some 296 times in my launch file and each time it was taking ~0.4 seconds, leading to a delay of almost 30 seconds:

Screenshot_2023-06-15_14-58-26

After the change in this pr, the profiler shows what I experience on the command-line, which is much better behavior, taking almost no time to start launching things, since we only look this up once:

Screenshot_2023-06-15_15-00-10

Some potential issues with this pr:

  • it will prevent extensions from changing after launch has started
  • we could potentially cache more of the function, but this was the minimal change that made a large difference

For the first item, I don't think the rest of the code is written to assume this is possible in the first place, and I'm not sure what the use case for this would look like. I suppose you could have launch running, and while running install new python packages that add extensions, then cause launch to start a new node after this, and in that case it would not see new extensions. However, I don't know when you'd do that in practice. And I think this change benefits the existing users at the cost of a potential use case that I do not believe folks are using today, but I'm open to hear otherwise.

For the second, I don't think we necessarily need to cache more, based on what I see in the second profiler.

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood self-assigned this Jun 14, 2023
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood added the bug Something isn't working label Jun 15, 2023
@wjwwood wjwwood marked this pull request as ready for review June 15, 2023 22:11
@adityapande-1995
Copy link
Contributor

adityapande-1995 commented Jun 15, 2023

IIUC, this means that if one installs packages (that add extensions) while launch is still spinning up nodes, the nodes that start after the installation is done won't reflect the changes, right ? If yes, that seems like a weird use case to me, as when the installation completes is not predictable, and nor does launch guarantee the order in which nodes start.

So yes a +1 to this change, looks good !

Would this cache also affect nested actions or launch files ? i.e. if a launch file includes another launch file will this cache be shared ?

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2023

Right, the first time a node asks for the list of extensions it is cached, so if you install something new in parallel, it will be a race to see if it is included or not and with which nodes. I can't imagine a use case for that anyway, but I wanted to point it out.

Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Looks good with green CI !

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

it will prevent extensions from changing after launch has started

i believe this is not the user expectation or assumption. lgtm.

@wjwwood wjwwood merged commit 13d6e1e into rolling Jun 16, 2023
3 checks passed
@wjwwood wjwwood deleted the wjwwood/cache_importlib_metadata_node_action branch June 16, 2023 21:37
asymingt pushed a commit to asymingt/launch_ros that referenced this pull request Jun 18, 2023
* cache lookup of importlib metadata in Node action

Signed-off-by: William Woodall <william@osrfoundation.org>

* style fixup

Signed-off-by: William Woodall <william@osrfoundation.org>

---------

Signed-off-by: William Woodall <william@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants