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

Improved zsh completion time from ~0.5s to near instant #483

Closed
wants to merge 1 commit into from

Conversation

racko
Copy link
Contributor

@racko racko commented Aug 16, 2017

The "catkin locate -s" takes around 0.5s. It gets called whenever the cache is checked.

@@ -102,7 +102,8 @@ _catkin_packages_caching_policy() {
if [[ -z "$_workspace_source_space" ]]; then
_debug_log "Searching for source space..."

_workspace_source_space="$(catkin locate -s --quiet)"
_catkin_get_enclosing_workspace $_workspace_root_hint
_workspace_source_space="$_workspace_root/src"
Copy link
Member

Choose a reason for hiding this comment

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

This is a flawed assumption, the source directory is not always <workspace dir>/src. Since it can be any folder, that's why catkin locate is required to look up the setting.

@racko
Copy link
Contributor Author

racko commented Aug 16, 2017

pkg_resources.require("catkin_tools")[0].version in migrate_metadata is "the problem":

time python2 -c 'import pkg_resources; pkg_resources.require("catkin_tools")[0].version'
python2 -c   0.14s user 0.01s system 99% cpu 0.148 total

Making migrate_metadata a no-op reduces the time for get_active_metadata to less than 30ms. Of course we can't do that.

Any idea if there is a way to replace pkg_resources.require("catkin_tools")[0].version with something (a lot) faster?

TODO: get_active_metadata and update_active_metadata are broken (but unused). They miss return keywords.


Well ...

import catkin_tools.context
ctx = catkin_tools.context.Context.load('/home/racko/catkin_ws', None, type('Options', (object,), dict(space='src'))(), load_env=False)
print ctx.source_space_abs

takes around 250ms.

from catkin_tools.metadata import get_metadata, get_active_profile
print get_metadata('/home/racko/catkin_ws', get_active_profile('/home/racko/catkin_ws'), 'config')['source_space']

takes around 175ms. Calling get_metadata or get_active_profile alone also takes around 175ms, so among other things, we can conclude that calling get_active_profile and then reading the config.yaml with another tool will not be good enough. But still it is interesting to measure.

import yaml
print yaml.load(open('/home/racko/catkin_ws/.catkin_tools/profiles/default/config.yaml'))['source_space']

takes around 30ms.

yaml2json - < /home/racko/catkin_ws/.catkin_tools/profiles/default/config.yaml | jq '.source_space'

takes around 20ms.

Also caching the source space path is not an option: To check if the cache is invalid, we need to check the date of the config.yaml, and getting the path takes around 175ms again ...

Reimplementing the get_active_profile logic in zsh (or some compiled language) would be trivial and fast enough, but duplicating the logic is of course not an option. And I'm certain that implementing the logic in C++ and using it in python (similar to tf) was decided against long before I came along, besides it being to big of a change for this pull request :)

What about adding a special case for "$_workspace_root/src" since it's what works for 99% of all users? It is the recommended layout after all ...

Not even this special case works: Finding "$_workspace_root/src" does not mean that it is the source space :(

- ... if the workspace has the recommended layout
@racko racko force-pushed the improved_runtime branch from bb26e81 to 04aa94a Compare August 16, 2017 20:59
@wjwwood
Copy link
Member

wjwwood commented Aug 17, 2017

If the issue is only due to asking for the version number, perhaps we could delay acquiring that information until it is asked for.

However, I think in general the entry points performance is a bigger issue, see: #297

@racko
Copy link
Contributor Author

racko commented Oct 13, 2017

I close this pull request because the proposed solution isn't actually working:

What about adding a special case for "$_workspace_root/src" since it's what works for 99% of all users? It is the recommended layout after all ...

Not even this special case works: Finding "$_workspace_root/src" does not mean that it is the source space :(

Also my suggestion that replacing pkg_resources.require("catkin_tools")[0].version would fix the problem seems to be false: While it certainly speeds up migrate_metadata, I couldn't observe a speed-up for catkin locate -s or the zsh auto-complete. I guess that pkg_resources.require("catkin_tools") or something similar gets called anyhow.

@racko racko closed this Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants