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

Universal variables re-narrow path again and again and again #5905

Closed
faho opened this issue May 30, 2019 · 3 comments
Closed

Universal variables re-narrow path again and again and again #5905

faho opened this issue May 30, 2019 · 3 comments
Labels
performance
Milestone

Comments

@faho
Copy link
Member

@faho faho commented May 30, 2019

In something like

for i in (seq 1000)
    command true
end

about 20% of the total time is taken up in wcs2string, much of that coming from universal_barrier() (which is triggered here because it's an external command - this does not happen with builtin true).

And that does wcs2string because it is narrowing the uvar path, again and again and again, even though it stays the same 99.999% of the time.

I propose we cache the narrow versions of these kinds of paths - we still need the wide version for output. Unfortunately this means changing from our nice and friendly "waccess" et al wrappers, because they do the narrowing for you.

@faho faho added the performance label May 30, 2019
@zanchey
Copy link
Member

@zanchey zanchey commented May 30, 2019

Should it be able to change while running at all? Does that make any sense?

@faho
Copy link
Member Author

@faho faho commented May 30, 2019

Should it be able to change while running at all? Does that make any sense?

Maaaayyybbee? Possibly if $XDG_CONFIG_HOME changes.

@faho faho added this to the fish-future milestone May 30, 2019
@faho
Copy link
Member Author

@faho faho commented May 31, 2019

Okay, no, we don't seem to change the uvar path currently.

It asks path_get_config, which in turn just returns a static variable:

static const base_directory_t &get_config_directory() {
    static base_directory_t s_dir = make_base_directory(L"XDG_CONFIG_HOME", L"/.config/fish");
    return s_dir;
}

faho added a commit that referenced this issue Jun 1, 2019
This cuts down on the wcs2string here by ~25%.

The better solution would be to cache narrow versions of $PATH, since
we compute that over and over and over and over again, while it rarely changes.

Or we could add a full path-cache (where which command is), but that's
much harder to invalidate.

See #5905.
@zanchey zanchey removed this from the fish-future milestone Sep 6, 2019
@zanchey zanchey added this to the fish 3.1.0 milestone Sep 6, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance
Projects
None yet
Development

No branches or pull requests

2 participants