Skip to content

Conversation

erikarvstedt
Copy link

Commit Clear envrc--status on mode exit is a minor correctness fix.

The other two commits are strict performance improvements and don't change runtime behaviour.
Both changes apply to the hot path of setting up a new buffer before it's displayed to the user, which is always worth optiziming.
I've run these changes on my system for the last few weeks.

This package is a joy to use, thank you for sharing it!

Instead of calling `envrc--merged-environment' on each buffer setup,
call it only once, after generating definitions with envrc,
and cache the resulting process environment with `envrc--cache`.

This saves needless variable formatting and merging and also allows
buffers to share instances of `process-environment'.
Only update cache for the updated process-environment after
calculating a new cache value.
Previously, the update happened on each buffer setup.

Remove use of pcase because cache values can never be nil.
purcell added a commit that referenced this pull request Nov 13, 2020
@purcell
Copy link
Owner

purcell commented Nov 13, 2020

Thanks! I committed a subset of this change: only updating the cache when necessary.

But as for merging the full environment and putting it in the cache instead of merging it each time a buffer initializes, I prefer to keep things the way they are: the merged environments can be much larger - particularly if they already contained direnv-outputted env vars - and merging them each time in envrc--apply is cheap. In fact, it's cheap because it leaves Emacs to do the "merging" of the "before" and "after" env vars, so that's also what makes the unmerged values bulky to store in the cache.

@erikarvstedt
Copy link
Author

Benefits of Optimize process environment creation:

  • Variable formatting (format "%s=%s" ...) and append is run only once per direnv setup and not for each buffer.
  • All buffers of a direnv share the same process-environment list. Without the optimization, the direnv-related prefix of the list is stored as a separate copy for each buffer, which can be significant for large direnvs and many buffers.

the merged environments can be much larger

The last argument to append, that is the existing process-environment, is not copied. So the default proc-env is shared between all cached envs.

particularly if they already contained direnv-outputted env vars

In the code you're making the explicit assumption that direnv is idempotent, so we're never prepending to a direnv-created proc-env anyways.

In my view, the optimization entails no added complexity and brings clear performance benefits.

New commit Don't update cache redundantly

This contains a bug. To add a cache entry for the new process-environment, as described in the comment, you have to add the entry with a cache key containing the new environment and not the old one. Previously, this entry was correctly added after the call to envrc--apply.

@purcell
Copy link
Owner

purcell commented Nov 14, 2020

The last argument to append, that is the existing process-environment, is not copied. So the default proc-env is shared between all cached envs.

No, process-environment might already be set buffer-locally by a package other than envrc.

This contains a bug.

Ah yes, true, I couldn't see why I had done it like that. Will revert.

@purcell
Copy link
Owner

purcell commented Nov 14, 2020

Going to have to think about this. The whole setup used to be much simpler, and then it ended up with additional complication in order to support buffer-local changes to process-environment, but I think at some point I may have concluded that those changes were not ideal, so I might revisit the entire approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants