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

Async all the segments #344

Closed
wants to merge 261 commits into
base: next
from

Conversation

Projects
None yet
9 participants
@dritter
Collaborator

dritter commented Nov 23, 2016

This PR changes all segments from synchronous to asynchronous

For now, this is just a proof-of-concept and at best an early alpha. So don't expect everything to work right on. There are already some known bugs (see below).
The code is still a mess, but I wanted to start off a discussion as early as possible.

So, what do you think?

Difference to earlier approaches

The key difference is that this PR executes all configured segments asynchronously instead of whole prompts. This has the benefit of re-rendering single segments (like for the vi-segment #319).

Zsh-async integration

Currently there is no zsh-async used in this PR. But I think zsh-async is a great library, that could deal with some of the Heisenbugs introduced with this PR (occasional shell crashes).. So, it is still a good idea to integrate zsh-async IMHO.

Technical description

  1. The User opens a new Terminal window
  2. We spin off various async processes, one for each segment
  3. We put the generated data into separate files (a cache) in the /tmp/p9k/ directory.
  4. For each prompt we generate the user specific overrides as well (maybe in the async process as well). At that point we already know in which state the segment is.
  5. Every time a async process finishes, we trap it, and refresh the prompt by reading the information generated in the cache files.
  6. The refreshing of the prompt is done by simply gluing the Segments together.

So the flow is separated in three parts:

  • The first part is where we fork the async child processes (step 1 - 2 from above). This happens in powerlevel9k_prepare_prompts (which is our precmd hook). In this function we call build_left_prompt and build_right_prompt, which invoke the prompt functions asynchronously.
  • Second part is the segments doing their heavy lifting (like calculating their result, checking the user set colors, etc.) and writing that to a cache file (step 3 - 4). This happens in the prompt_* functions. At the end they call serialize_segment which resolves the user overrides and writes it all to cache files.
  • The last step is the async processes coming back. We read the cache files in and glue the segments together. This step should be as fast as possible (step 5 - 6). Our trap function is TRAPUSR1, which calls p9k_build_prompt_from_cache. In that function all segments are glued together. The trap is called for each segment exiting its child process. So, it gets as often called as you have segments configured. That is the reason why it should be as fast as possible.

Known bugs

There are still some things missing:

  • Enable async functionality on all segments
  • Re-enable joining segments together (currently disabled)
  • Re-enable multi-line prompt (currently only single line)
  • Re-enable ability to disable the right pompt
  • Re-enable VI-mode segment
  • Check custom segments (should work, but untested)
  • Change signal to SIGWINCH (see zsh-async)
  • Proper cache cleanup
  • Silence current cleanup task (see p9k_cleanup_cache)
  • Handle contentless segments (these won't get printed now and prevent proper joining)
  • Check transitive joined segments with a conditional one in between
  • Check joining segments that miss their condition (cache file still has content)
  • Occasional crashes of the whole shell
  • Fix Prezto
  • Fix setopt PROMPT_SUBST in Prezto (needs to be set manually ATM)
  • Fix zplug
  • Fix zplugin in VM (currently we link the theme, but that is not how zplugin itself does it)
  • Fix tests
  • Add more tests
  • Add ZPM to VM
  • Add ZGen to VM
  • Add Prezto-Community-Fork to VM (very recent fork of Prezto
  • Fix occasional not rendering of segments
  • Re-Fix the p3wnage bug
  • Merge next into this branch

Optional:

  • Revive some ideas of the "Improve Ruby version"-Branch (#119)
  • Add ZSH-Async
@bhilburn

This comment has been minimized.

Owner

bhilburn commented Nov 28, 2016

Hey @dritter! Good to have you back =) And wow, what an entrance, hah!

@andjscott @Tritlo @wadkar

This is awesome, especially given the discussion in #331 about integrating async into PL9k.

Your workflow seems well thought-out. It also sounds like you think migrating what you did to zsh-async is the right path forward, is that accurate?

@Tritlo

This comment has been minimized.

Collaborator

Tritlo commented Nov 29, 2016

Nice, I look forward to trying this out when it is a bit further along. I agree that we should use zsh-async instead of some homebrew, if only for all the free support we'd get. Rendering each segment also seems like a good approach, since it is often the case that there is only a few segments that are slow. We should however have some placeholders, so as if to not jar the display too much when the information finally comes through.

@wadkar

This comment has been minimized.

Collaborator

wadkar commented Nov 29, 2016

dritter added some commits Nov 29, 2016

Add a method that can find the index of an element in an array
This can be used to find all indices of a segment configured in left or
right prompt elements.
@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 29, 2016

I just re-enabled the vi_mode. :)

@bhilburn Yep, absolutely. I just had not enough time to play around with zsh-async to know it will fit our needs. But I'll try to integrate it, once the other missing features in this branch are reimplemented again.

@Tritlo yep. It is still a very early peek. Thanks for your comments.

@wadkar Cool! Thanks for the offer 👍 Much appreciated.

@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 29, 2016

Here is a little gif comparison in a repo with 500k untracked files.

Current next:
p9k_0 5 0_big_repo

This branch (with vi_mode):
p9k_async_in_big_repo
The segments flicker a bit and occasionally disappear (this will get hopefully better with zsh-async).

@cbourgeois

This comment has been minimized.

cbourgeois commented Nov 30, 2016

@dritter I just pulled your branch to give it a go, appart from some flicker it is very promising. I just had to change the ls -1 --color=never to ls -1 on macOS because it is not supported by the ls binary there.

@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 30, 2016

Wow. There are a lot of typos in the gifs.. :D

@cbourgeois good catch! I want ls to always be colorful, so I aliased it to ls --color=always which is not available on OSX default ls.. But that setting breaks the use of ls in the theme, and that is the reason why I disabled the colored output there.. Long story short: I changed my local settings and removed it from the theme.

dritter added some commits Nov 30, 2016

@Tritlo

This comment has been minimized.

Collaborator

Tritlo commented Dec 4, 2016

Looks very promising, but as I say, I'd prefer the segment to be shown partially (maybe just the icon for it, like the GitHub Octocat?) rather than have it invisible until it can be fully rendered.

Tritlo and others added some commits Dec 4, 2016

Merge pull request #1 from Tritlo/async_all_the_segments
Fix > not working on OSX without rm first
Fix anaconda segment
- Before, there were no default colors set.
- Fix variable scope
@Tritlo

This comment has been minimized.

Collaborator

Tritlo commented Dec 4, 2016

Hmm. After looking at the code and pondering it for my self, I don't think that my suggestion is plausible. We don't really know if the segment will render at all before we try to get the output, which is what takes a long time.

@Tritlo

This comment has been minimized.

Collaborator

Tritlo commented Dec 5, 2016

So I made an attempt at integrating zsh-async, as can be seen in Tritlo/powerlevel9k@b16dab5. However, I hit a pretty serious snag: it seems as if zsh-async and its async_job does not implement closure passing, i.e. when I try to call e.g. prompt_os_icon asynchronously, I get an error that reads (eval):1: command not found: prompt_os_icon. This makes it appear as if the asynchronous call is not made from the same file, and cannot reference variables or functions from the file. We could sidestep this by making every function self-contained in a separate file and then run that file as a script as an async job, but that seems like quite the hassle, seeing as our current approach seems to work ok.

@onaforeignshore

This comment has been minimized.

Contributor

onaforeignshore commented May 4, 2017

The post was made 5 years ago. I think a lot has changed since then. We redraw the whole prompt anyway, so I don't think it makes a difference compared to what he's talking about.

@robobenklein

This comment has been minimized.

Contributor

robobenklein commented Jun 19, 2017

Having a different issue I didn't see in your bug list:
broken terminal prompt

Not sure where to start debugging this at all.

    POWERLEVEL9K_LEFT_PROMPT_ELEMENTS=(context dir_writable dir vcs)
    POWERLEVEL9K_RIGHT_PROMPT_ELEMENTS=(status command_execution_time root_indicator background_jobs battery time)
    # Specifics
    POWERLEVEL9K_BATTERY_LOW_THRESHOLD=30
    POWERLEVEL9K_COMMAND_EXECUTION_TIME_THRESHOLD=1
    POWERLEVEL9K_CONTEXT_DEFAULT_FOREGROUND="white"
    POWERLEVEL9K_BATTERY_ICON=""
    POWERLEVEL9K_SHORTEN_DIR_LENGTH=1
    POWERLEVEL9K_SHORTEN_DELIMITER=""
    POWERLEVEL9K_SHORTEN_STRATEGY="truncate_from_right"
    POWERLEVEL9K_COMMAND_EXECUTION_TIME_BACKGROUND="black"
    POWERLEVEL9K_COMMAND_EXECUTION_TIME_FOREGROUND="242" # dimm gray
    POWERLEVEL9K_EXECUTION_TIME_ICON=""

timings test debug script

@onaforeignshore

This comment has been minimized.

Contributor

onaforeignshore commented Jun 19, 2017

get_indices_of_segment is in functions/utilities.zsh. Check if the file is in that folder and that it is not corrupted.

@robobenklein

This comment has been minimized.

Contributor

robobenklein commented Jun 19, 2017

It's definitely there:
image

@dritter

This comment has been minimized.

Collaborator

dritter commented Jul 12, 2017

@robobenklein Sorry for the late reply. Do you still get the error?

@dritter

This comment has been minimized.

Collaborator

dritter commented Jul 12, 2017

Update:
I spent the last nights debugging the occasional not rendering of segments. Unfortunately this seems to be a bug in ZSH. If I have segments A, B and C, and I start a new ZSH, probably all segments get rendered. Then I start to press enter with a short delay to avoid cancelling out running subprocesses, until a segment does not get rendered. Let's say that disappeared segment is B. Now, if I look into the cache files for that process (from another shell of course), it says that segment B was rendered, which leads me to my assumption that the subprocess for B did not come back / was cancelled by ZSH.

I tried to dig into the ZSH source, but my C is a bit rusty (no pun intended and quite an understatement - it's basically not existent). My weak theory at this point is that we exceed the internal queue size for traps. But to be sure I need to compile my own ZSH (see INSTALL instructions).

Long story short: I would love to see someone with more C skills to have a look on this.

@robobenklein

This comment has been minimized.

Contributor

robobenklein commented Jul 12, 2017

Yeah, still broken for me.
image

docwhat and others added some commits Jul 13, 2017

editorconfig: trim trailing spaces
- Fixed the issues in test/core/prompt.spec
- Added `root = true`
- Added URL on info about EditorConfig
Merge pull request #3 from docwhat/pr/dritter-async/cache_dir
Make CACHE_DIR safe and XDG compliant
@dritter

This comment has been minimized.

Collaborator

dritter commented Jul 17, 2017

@robobenklein Strange. Could you check the last commit you are on?

@robobenklein

This comment has been minimized.

Contributor

robobenklein commented Jul 19, 2017

Just tested 27c1a8a

Got it working by not loading the normal p9k once. Although it is behaving differently/incorrectly,

Here's the difference, with both themes given the same P9K variables:
image

Depicted:

  • Username prompt color not correct
  • Shows exit status zero for some reason
  • On the which y line, apparently the dir segment died?

dritter added some commits Jul 28, 2017

Remove guard that should kill orphaned child processes
This does not work, as we just saved the PID from the last child that
was executed. To work correctly it needs to check ALL child processes,
but I am not sure if such a guard is necessary.
@ghost

This comment has been minimized.

ghost commented Nov 3, 2017

This is almost a year old. What's the hold up?

@bhilburn

This comment has been minimized.

Owner

bhilburn commented Nov 3, 2017

@nate0001 - We are all quite busy, and this effort is very significant. @dritter has written his own async library for ZSH in the process of making this work. If you're asking for areas where you might be able to contribute, @dritter is the best person to point you in the right direction =)

@krostar

This comment has been minimized.

krostar commented Mar 7, 2018

Hi,

I have the same issues (slow prompt, ctrl+c sometimes and some errors are displayed).
The solution explained above (using dritter fork) is broken (no prompt_user function, when I stop using user element no theme is displayed at all (maybe I'm doing something wrong)).

I know it's a hard subject (based on this message for example), I'm not an ZSH expert but if I can do something, what can I do to help ?

@bhilburn

This comment has been minimized.

Owner

bhilburn commented Mar 7, 2018

Hey @krostar - Thanks so much for offering to help! I really appreciate it, and we could definitely use your assistance. We are actually starting to hack on some major new developments in the next branch to address these issues (among others), and will need a lot of help testing it out and reviewing. If you're up for it, I'll pull you into some of those efforts =)

This was referenced Jun 4, 2018

@dritter dritter referenced this pull request Jun 28, 2018

Merged

Add GitHub templates #886

@dritter dritter referenced this pull request Jul 27, 2018

Merged

Add tests #934

@dritter

This comment has been minimized.

Collaborator

dritter commented Nov 12, 2018

Closing this PR. A lot of speed improvements has landed in the next branch. We are working on implementing async functionality based on zsh-async currently.

@dritter dritter closed this Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment