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

Use powerlevel10k instant prompt with zim+ #11

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

ericbn
Copy link
Contributor

@ericbn ericbn commented Dec 14, 2021

Zim does play well with powerlevel10k instant prompt. It also "has one function to install plugins and another to load them", contratry to what the zsh-bench README.md mentions.

@ericbn
Copy link
Contributor Author

ericbn commented Dec 14, 2021

@romkatv, you might want to update the README.md accordingly.

Also, regarding the README.md, the information about Zim being optimized on the exit metric is now outdated, since we recently updated our benchmark to measure the time to the first prompt instead. Can you please update that too?

Zim does play well with powerlevel10k instant prompt. It also "has one
function to install plugins and another to load them", contratry to what
the zsh-bench README.md mentions.
@romkatv
Copy link
Owner

romkatv commented Dec 14, 2021

Thanks! Merged.

@romkatv, you might want to update the README.md accordingly.

Of course. Will do that tomorrow or the day after.

Also, regarding the README.md, the information about Zim being optimized on the exit metric is now outdated, since we recently updated our benchmark to measure the time to the first prompt instead. Can you please update that too?

Ditto.

@ericbn ericbn deleted the zim+ branch December 14, 2021 21:15
romkatv added a commit that referenced this pull request Dec 15, 2021
… zim now uses a meaningful performance metric instead of 'exit' (#11)
@romkatv
Copy link
Owner

romkatv commented Dec 15, 2021

I've updated the readme.

I've also tried to change zim+ config so that it prints a notification when zim is installing something. I think it's bad UX when network IO -- which can take arbitrary long -- is done while the screen is empty. What's the right way to do that? I tried removing -q from init but that causes a screenful of text output (looks like a help message?). It appears that replacing init with install mostly fixes this issue except for the misleading and unhelpful message at the end that reads "Done with install. Restart your terminal for any changes to take effect.". The latter message is sent to stdout, just like the "installing" notifications that I want to preserve, so I cannot use file redirection to suppress the unwanted message. I ended up doing this:

source $ZIM_HOME/zimfw.zsh install |
  grep -vF 'Done with install. Restart your terminal for any changes to take effect.'

Is there a better way?

@ericbn
Copy link
Contributor Author

ericbn commented Dec 15, 2021

This is the output I get with source ${ZIM_HOME}/zimfw.zsh init without the -q:

) completion: Installed
) zsh-autosuggestions: Installed
) zsh-syntax-highlighting: Installed
) powerlevel10k: Installed
Done with install. Restart your terminal for any changes to take effect.
) /home/tmp/.zim/init.zsh: Updated. Restart your terminal for changes to take effect.
) /home/tmp/.zim/login_init.zsh: Updated. Restart your terminal for changes to take effect.
Done with build.

I agree the "Restart your terminal for any changes to take effect." does not make sense here. And the info on the build of the scripts may be too much information. I'll fix this in the next release.

@romkatv
Copy link
Owner

romkatv commented Dec 15, 2021

What's the difference between init and install? I used install because it's listed in source $ZIM_HOME/zimfw.zsh help (unlikely init).

@ericbn
Copy link
Contributor Author

ericbn commented Dec 15, 2021

init is intended to be used in .zshrc so we can keep the functionality separate from install, which is intended to be used in the command line. Currently the difference is that init does not do the compile action (which install does), because the compile is done in the background every time .zlogin is sourced.

EDIT: I'll add init to the zimfw help in the next release too, so this is clearer.

@romkatv
Copy link
Owner

romkatv commented Dec 15, 2021

Currently the difference is that init does not do the compile action (which install does), because the compile is done in the background every time .zlogin is sourced.

So install is what I need, right? It's nice to compile things right away to lower the chance of corruption by the background process.

@ericbn
Copy link
Contributor Author

ericbn commented Dec 15, 2021

The way the Zim startup is intended to work is:

  • install new modules and rebuild the init scripts (init.zsh and login_init.zsh) if the .zimrc file changed, via zimfw init, in .zshrc
  • source init.zsh to initialize modules, in .zshrc
  • always recompile scripts in background via source login_init.zsh, in .zlogin

The recompile happens independently, because we also want to recompile after some module was updated, or after the user changed some of its scripts manually.

Not sure if recompiling the scripts in .zshrc (using zimfw install, instead of init) and also doing the same in parallel (using source login_init.zsh) might have the risk of concurrency issues. Also, we might change what init does in the future, according to what we want to trigger during the Zim startup.

@ericbn
Copy link
Contributor Author

ericbn commented Dec 15, 2021

Using install instead of init will also further delay the instant prompt, which we don't want to do.

@romkatv
Copy link
Owner

romkatv commented Dec 15, 2021

Using install instead of init will also further delay the instant prompt, which we don't want to do.

Only when installing. It's fine. Besides, fetching data from the internet is much slower than compiling, so there is barely any difference.

I think I'll keep install as it seems to work. It doesn't completely eliminate the risk of the background process corrupting files but it's still better than init. The background process can only corrupt files when it's compiling them, so reducing the number of files that it compiles is a win.


What follows is unsolicited feedback that's probably 50% misguided. Feel free to ignore at will.

The background process corrupts files in several cases. Here are a few examples:

  1. You ssh to a remote server and log out quickly while the background process is running zcompile. The background process gets killed by SIGHUP, which leaves corrupted *.zwc files around. These files will be read the next time you login and you'll get errors. You might think that masking SIGHUP (e.g., with nohup(1)) would fix this but it won't. On systems with systemd those processes will still be killed. Systemd is found on many (perhaps most) servers, so it's not some weird corner case.
  2. You open one terminal tab and then quickly open another one. The second tab might be unlucky enough to see *.zwc files that are still being written by the first tab. Reading those files will cause errors.

The right solution is to not use the background process. It doesn't even improve performance, it only makes things brittle. I think the overall effect on performance is negative 0.5ms. That is, it makes things slightly slower.

As far as compiling goes, here's the way (IMO):

  1. Do not compile user rc files. Correctness first, performance second. If some users know what they are doing and want to compile their rc files, it's on them to deal with the fallout but it shouldn't be the default.
  2. Compile plugins after the installation.
  3. Compile zcompdump when it changes.

@ericbn
Copy link
Contributor Author

ericbn commented Dec 15, 2021

Lots of valuable feedback in there, thanks a lot! I'll experiment with it. It's even better to do experiments now that we have a more reliable benchmark in Zim too.

At the end, I might change init to do exactly what install does, and remove the background compiling.

ericbn added a commit to zimfw/zimfw that referenced this pull request Jan 7, 2022
Changes are:
  * Don't compile in the background anymore, only via the `zimfw` tool
    after actions where scripts can change (install, update, etc.)
  * Don't compile users startup scripts anymore (.zshenv, .zshrc, etc.)
  * Make output of `zimfw init` friendlier for the terminal startup
    screen when called without `-q`.

See romkatv/zsh-bench#11 (comment)
ericbn added a commit to zimfw/zimfw that referenced this pull request Jan 10, 2022
Changes are:
  * Don't compile in the background anymore, only via the `zimfw` tool
    after actions where scripts can change (build, install, update)
  * Move compilation of the completion dumpfile to the completion module:
    https://github.com/zimfw/completion/blob/9386a76eac3f55b1c04d57d26238f725b4b3ba25/init.zsh#L10-L11
  * Don't compile users startup scripts anymore (.zshenv, .zshrc, etc.)
  * Make output of `zimfw init` friendlier for the terminal startup
    screen when called without `-q`.

See romkatv/zsh-bench#11 (comment)
and https://github.com/romkatv/zsh-bench#cutting-corners

Regarding not compiling users startup scripts anymore, I'm choosing to
only compile the modules' scripts at least for the reason that compile
won't happen so ofter anymore -- it will only happen when the user calls
the `zimfw` build, install or update actions. So it makes more sense to
only compile the files that `zimfw` has control over changes...

Closes #450
ToddMenier added a commit to ToddMenier/zimfw that referenced this pull request Dec 26, 2022
Changes are:
  * Don't compile in the background anymore, only via the `zimfw` tool
    after actions where scripts can change (build, install, update)
  * Move compilation of the completion dumpfile to the completion module:
    https://github.com/zimfw/completion/blob/9386a76eac3f55b1c04d57d26238f725b4b3ba25/init.zsh#L10-L11
  * Don't compile users startup scripts anymore (.zshenv, .zshrc, etc.)
  * Make output of `zimfw init` friendlier for the terminal startup
    screen when called without `-q`.

See romkatv/zsh-bench#11 (comment)
and https://github.com/romkatv/zsh-bench#cutting-corners

Regarding not compiling users startup scripts anymore, I'm choosing to
only compile the modules' scripts at least for the reason that compile
won't happen so ofter anymore -- it will only happen when the user calls
the `zimfw` build, install or update actions. So it makes more sense to
only compile the files that `zimfw` has control over changes...

Closes #450
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