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

Slow when typing ** [condition: shopt -s globstar] #82

Open
7 of 8 tasks
3ximus opened this issue Jan 21, 2021 · 28 comments
Open
7 of 8 tasks

Slow when typing ** [condition: shopt -s globstar] #82

3ximus opened this issue Jan 21, 2021 · 28 comments

Comments

@3ximus
Copy link

3ximus commented Jan 21, 2021

Hi there,

I'm having some problems when typing **, specially on large directories. I assume this could be something to do with fzf feature of **, however I don't have this feature enabled (I think) . In this example I'm typing and it takes too long to write what I typed:

double_star

In cases of very large directories (with lots of entries) it will hand the shell.
Is it possible to disable this behavior by default ?


List of Discussed Issues

Edit by @akinomyoga

  • Highlighting blocked by globstar
  • Completion blocked by globstar
  • Highlighting with globstar significantly slower than command execution
    • Caused by multiple pathname expansions for each path segment
  • Input is slow even with bleopt highlight_timeout_sync=0 by async highlighting
    • Related to the cumulated timeouts in one pass of highlighting
  • Slow TAB completion with globstar highlighted already
  • Crash/hang on TAB completion
    • Unknown condition. Reproduced a few times at @akinomyoga side
    • conditional-sync is suspicious because there is always a subshell when it occurred.
  • Tilde prevents pathname expansions for highlighting
  • Input/completion speed is linked with the size of expanded results
@akinomyoga
Copy link
Owner

Thank you for the report! OK, I think it is related to the setting shopt -s globstar.

There are several possible bottlenecks, which I'll later investigate. ble.sh internally performs pathname expansions for syntax highlighting, auto-complete, etc. To find which part is the bottleneck, could you test if the response time changes with each of the following settings?

  • Q1. Does the situation changes with shopt -u globstar?
  • Q2. Does the situation changes with bleopt highlight_filename=?
  • Q3. Does the situation changes with bleopt complete_auto_complete=?

To solve the problem, now I'm thinking of running pathname expansions containing ** in subshells with some timeout. But this solution requires additional computational cost for forking the subshell and the busy-weight polling for the subshell termination. If you have any other idea, please feel free to let me know it!

@3ximus
Copy link
Author

3ximus commented Jan 21, 2021

* Q1. Does the situation changes with `shopt -u globstar`?

Yup, unsetting that option makes it fast again

* Q2. Does the situation changes with `bleopt highlight_filename=`?

Yup, this stops the issue

* Q3. Does the situation changes with `bleopt complete_auto_complete=`?

This doesn't fix it...


To solve the problem, now I'm thinking of running pathname expansions containing ** in subshells with some timeout. But this solution requires additional computational cost for forking the subshell and the busy-weight polling for the subshell termination. If you have any other idea, please feel free to let me know it!

So I guess it's a filename coloring problem, so a quick fix would be to just disable completion highlighting for this case?

Idk how you are currently getting the files to serve to the autocomplete, but would it be possible to run it in a sub-shell and continuously update the suggestions based on the filter as soon as it gets more values? For example similar to how fzf handles its file search.
Like only when you type **/<TAB> it would launch this sub-process, anything typed afterwards would be a filter.
Currently it doesn't really work correctly since it just shows suggestions for the first directory, so something like this could be an improvement.

If this works it could be applied to all file suggestions, and it would also fix the problem when trying to request completions for a directory with thousands of files.

I don't think a timeout is necessary in this case, maybe eventually some indicator that the search job is running would be good, and the first SIGINT could be sent to it if needed instead of aborting the current command.

And highlighting could be performed only for the results being displayed instead. Idk if you are doing this already...

Idk how polling for these new incoming suggestions would work though... How are you currently displaying the suggestions?

I could help you out on testing this but I would need a few pointers to where some of these things are, like generating filepahts, displaying suggestions and filtering them. Maybe I could at least experiment some ideas... and probably hit the same problems you already did xD.

Tell me what you think since this certainly brings a lot of issues I'm not seeing.

@akinomyoga akinomyoga changed the title Slow when typing ** [condition: shopt -s globstar] Slow when typing ** Jan 22, 2021
@akinomyoga akinomyoga changed the title [condition: shopt -s globstar] Slow when typing ** Slow when typing ** [condition: shopt -s globstar] Jan 22, 2021
@akinomyoga
Copy link
Owner

akinomyoga commented Jan 23, 2021

Thank you for the tests! I think, at least, one of the problems occurs inside the highlighting of filenames. In your quick test, it seems auto-completion didn't suffer from the problem, but I think it can also be blocked by slow ** pathname expansion at a certain condition.

So I guess it's a filename coloring problem, so a quick fix would be to just disable completion highlighting for this case?

The suggested solution seems to be not so easy to implement soon, so for a quick fix, I think I'll introduce the above workaround (subshell & timeout) for the patterns that contain raw **.


Idk how you are currently getting the files to serve to the autocomplete, but would it be possible to run it in a sub-shell and continuously update the suggestions based on the filter as soon as it gets more values? For example similar to how fzf handles its file search.

That is indeed an interesting idea! I have considered that direction. It seems some technical problems should be solved to implement it in the shell.

Below are some random thoughts on those problems. Edit: I think Problems 2 and 3 can be handled if we really think this idea valuable and make sufficient efforts. The real problem is Problem 1.

Problem 1. Incremental Pathname Expansions

First, ble.sh uses Bash's pathname expansion to generate the file list from the pathname expansion using the constructs something like array=(**), func **, or for x in **. The problem is that there is no way to perform incremental pathname expansion, i.e., no way to get the partial results of Bash's pathname expansion incrementally. array=(**) and func ** completes with a complete list only after the Bash scanned all the directories and collected all the filenames.

  • Other Implementation: We need to rely on some external command that supports Bash's dialect of globbing patterns and generates the pathname expansion results incrementally. If there isn't such a command, we may write an emulator of Bash's pathname expansions by ourselves in a Bash script, but I think it would be extremely slow.
  • Perfect Compatibility: If we perform the pathname expansions with a third-party implementation, that implementation should be completely compatible with Bash's implementation, or otherwise the wrong highlighting would confuse users. I don't want to introduce an incomplete implementation that provides wrong information to users.
  • Interaction with Other Expansions: In any case, there are many non-trivial things for which we need to carefully investigate Bash's behavior; Bash's pathname expansion would be performed as merely one part of the shell expansions, and the interference between the other shell expansions is non-trivial (e.g., consider $pat**"${arr[@]}" or something). When we perform the pathname expansions without using the original implementation of Bash's pathname expansion, we also need to emulate all these interferences with other expansions.

Problem 2. Slow Bash

Next, Bash is slow. In my expectation, as far as it is implemented in a Bash script, we cannot expect as responsive user experience as fzf which is implemented in Go Rust.

  • Fork cost: Creating a subshell itself also has a considerable cost. In particular, fork in Windows-based systems (WSL, Cygwin, MSYS, etc.) has a large cost. For this reason, I want to avoid forking subshells for each keystroke if possible. Maybe we can keep subshells alive as background workers as in thread pool, but this also requires
    complicated control on synchronization.
  • Expansion cost: Although we can respond to the user input quickly by delegating the pathname expansions to subshells, subshell doesn't shorten the time to get the result of the pathname expansions.
  • Communication cost: If the pathname expansions are performed in the subshell, the result needs to be passed to the parent shell using some way through pipes or temporary files. This also requires additional computational cost.
  • Filtering cost: If one doesn't restart the subshell for each keystroke but instead continues to use the subshell by treating the additional inputs as a filter, we also need to think about the cost of the filtering. I guess this would be the largest bottleneck.
  • Rendering cost: If one continuously updates the word list as the subshell gets more filenames, we also need to think of the cost of the rendering of the word list in the terminal. In showing the word list in the terminal, ble.sh calculates the layout of the words, determine the color and style of the words, and construct the escape sequences to print words, which also requires some time to process.

Problem 3. Implementation Complication

Also, full support of such a continuous implementation would lead to the complication of the implementation. Bash is a single-thread language and doesn't provide first-class support of alternative mechanisms such like coroutines or fibers. Bash doesn't provide first-class support of semaphore, mutex, or other facilities for the synchronization. These language limitations make it difficult to do asynchronous programming in Bash. We may still implement these mechanisms by hand, but it will require a big rewrite of the existing codes, and also the code would become unnecessarily complicated. We may create background processes by command & or coproc, but these are independent processes which aren't so useful as threads in other languages.

  • Interprocess Communication (IPC): Since subshells are independent processes, variable namespaces are not shared with the parent shell, so one needs to think about how to transfer the results in the subshell to the parent shell. If we want to do the continuous updates, one cannot use the form result=$(subshell) since it blocks the execution of the parent until the subshell terminates. We may use named pipes but in some systems (such as in Cygwin), there is no full support of the named pipes such that it allows IPC. We may use coproc, but it is not available in Bash 3. We may use temporary files (i.e., subshells write bytes to the temporary file, and the parent reads out the bytes from the file), but it requires careful control on the synchronization.

  • Asynchronous Processing: As already written, there are no threads or coroutines in Bash. If one wants to write asynchronous processing in Bash, one needs to explicitly divide each step of the processing in functions, save and restore local variables, etc.

    Actually, ble.sh has already implemented such kind of processing for command history initialization, history search, other initializations, auto-complete, and menu-filter. However, it was possible because these are relatively simple asynchronous tasks which don't have complicated internal states and also don't have many side effects on other functionalities.

    For syntax highlighting, the internal state is really complicated, so I guess it will be a big rewrite, but I'm not sure if it's worth rewriting for just determining the color of the argument. For auto-complete, I guess it is not so difficult to make it asynchronous compared to the syntax highlighting.


Misc

I don't think a timeout is necessary in this case, maybe eventually some indicator that the search job is running would be good, and the first SIGINT could be sent to it if needed instead of aborting the current command.

I have been also thinking that a timeout or some hard-coded limitation is unnecessary for background tasks, but in the discussion at #64 (comment) it was pointed out that the fact Bash occupies so much CPU usage in the background for any user input may be unpreferable for many users.

In particular, the pathname expansions like ** results in the scan of the entire directory tree, it can occupy disk I/O for a long time, which slows down the whole system. If it were explicitly invoked by user operation such as through fzf, maybe it's not a that big issue as it is not so frequent and the user can control that. But it may be too heavy operation to be run in the background automatically.

And highlighting could be performed only for the results being displayed instead.

I'm sorry that I couldn't get what you mean by this? Would you mean stopping filename highlighting for the words that contain special glob characters?

Idk how polling for these new incoming suggestions would work though... How are you currently displaying the suggestions?

Currently, in the processing of auto-complete, we constantly checking stdin (user inputs) if it has a new byte. When we find a new byte in stdin, we cancel the processing of auto-complete and process the user input instead. After that, we restart auto-complete from the beginning.

I could help you out on testing this but I would need a few pointers to where some of these things are, like generating filepahts, displaying suggestions and filtering them. Maybe I could at least experiment some ideas... and probably hit the same problems you already did xD.

Completions are implemented in lib/core-complete.sh.

  • The pathname expansions for completions are performed in the function ble/complete/util/eval-pathname-expansion.
  • To display the suggestion, ble.sh first enters a special state auto-complete-mode (here) and also inserts suggested texts into the command line (here). These temporarily inserted texts are removed when ble.sh exits the auto-complete-mode (here). The rendering of inserted texts is performed using the normal routines to show the command line (with additional region highlighting by the auto_complete face).
  • To display the menu, the rendering content is generated in the function ble/complete/menu#construct. The actual layout and output content is implemented in ble/complete/manu-style:MENU_STYLE/construct-page called from here. Here, MENU_STYLE specifies a style of the menu: you can find implementations at e.g. align, dense, linewise, desc.
  • Filtering of the candidates in the menu is performed by the shell function ble/complete/menu-filter/.filter-candidates.

@3ximus
Copy link
Author

3ximus commented Jan 25, 2021

Indeed this brings quite a lot of problems...
Thanks for the guidelines, I'm a bit busy at the moment but when I find some time I'll give this some thought and experiment a bit and I'll get back at you, maybe in a few days.

In the meantime do you think it's possible to do some workaround to prevent it being so slow, because even when scrolling through the command history it halts for a while when it finds a command with ** .

@akinomyoga
Copy link
Owner

akinomyoga commented Jan 25, 2021

In the meantime do you think it's possible to do some workaround to prevent it being so slow, because even when scrolling through the command history it halts for a while when it finds a command with ** .

This is rather related to the syntax highlighting but not to auto-complete. In the previous comment, I have shown the pointers for auto-complete but not for the syntax highlighting. If you want the pointers for the syntax highlighting as well, please ask me again. Also, I guess you'll get further questions on the code of auto-complete, so also please feel free to ask me about them.

In these couple of days, I too have thought about how we can manage this problem in the syntax highlighting. Now I'm thinking of performing word highlighting as a background task (but not in a subshell). Maybe it requires a large change of the code, but I believe it is possible.

@akinomyoga
Copy link
Owner

In the meantime do you think it's possible to do some workaround to prevent it being so slow, because even when scrolling through the command history it halts for a while when it finds a command with ** .

In spike branch, I have implemented the subshell evaluation of pathname expansions with user-input cancellation and timeouts for syntax-highlighting 984662e and completion 1ab50a8 together with a workaround of a related problem in Cygwin 668e231. I also implemented caching of the expansion results 1e771d5.

Now I don't notice remaining problems with these changes, but they are a relatively large set of changes (47a3301...1e771d5) and haven't been tested well, so I haven't yet pushed it to the master branch. If you are interested, could you test it by checking out the spike branch?

Thank you

@3ximus
Copy link
Author

3ximus commented Jan 29, 2021

Hi there,
Awesome!! I tried and it seems a lot faster! It still hangs a bit on each key-press, does user-input cancellation work with a timeout ?
I'll use this branch for now and report any problem that I find.
Thank you!

@akinomyoga
Copy link
Owner

akinomyoga commented Jan 29, 2021

It still hangs a bit on each key-press, does user-input cancellation work with a timeout?

The user-input cancellation and timeouts are independent, but I actually guess the delay you see here is rather related to a timeout.

Let me explain the behavior. For example, consider the case you input **/*ab followed by c. In this case,

  1. ble.sh renders **/*ab,
  2. append c in the internal buffer so that the internal string becomes **/*abc (note that **/*abc is not yet shown in the terminal at this point),
  3. and try to perform the pathname expansion on **/*abc for highlighting. This takes much time. It times out after highlight_timeout_defer milliseconds.
  4. ble.sh renders **/*abc in the terminal (without highlighting because the highlighting process has timed out),
  5. and again retry to perform the pathname expansion on **/*abc in background. This times out after highlight_timeout_cancel milliseconds.
  6. Finally it rerenders **/*abc with highlighting (if the highlighting is successfully completed within the timeout).

The user-input cancellation instantly occurs any time when you input additional character d in the above step 3 or 5.

Maybe we want to make the timeout highlight_timeout_defer (whose default is 500 milliseconds for now) shorter. For example, you can try the following for the 50-millisecond timeout.

bleopt highlight_timeout_defer=50

Thank you!!

@akinomyoga
Copy link
Owner

akinomyoga commented Jan 30, 2021

I rebased and force-pushed spike branch on top of the latest version of master branch (for the new feature bleopt emoji_version). I have also added following changes:

  • The option name highlight_timeout_defer has been changed to highlight_timeout_sync.
  • The option name highlight_timeout_cancel has been changed to highlight_timeout_async.
  • The default value of highlight_timeout_sync was changed from 500 to 50 after your feedback.

@3ximus
Copy link
Author

3ximus commented Jan 30, 2021

Hey there,
Yeah, its working much better, and I think I would even lower the highlight_timeout_sync to something like 20...
What are the benefits of even having this sync timeout though? If you manage to implement the async highlighting why not just use that? I notice that setting this timeout to zero makes typing after the globstar pattern much worse though...

Also after typing a globstar path and even after this has been correctly highlighted, asking for completion with <tab> on something typed afterwards gets really slow.

These improvements are really good, but I'm wondering if it would be simpler to just ignore highlighting on glob patterns since they might not even be accurate and not really relevant. For example in path/**/*.sh it doesn't really matter if *.sh is highlighted, since not all .sh files might be executable... Also in some/**/path*/* not all path directories might have the same highlighting. And these more complex patterns take much longer than running ls with the same pattern, and typing something afterwards becomes terrible...
So overall, I think the benefits of highlighting these expressions might not be worth it.... Maybe just ignoring the globstar patterns on highlighting would be better overall...

@akinomyoga
Copy link
Owner

akinomyoga commented Jan 31, 2021

I think I would even lower the highlight_timeout_sync to something like 20...

Yeah, I actually thought something shorter time is enough for the quick pathname expansions, but unfortunately, the fork cost for subshells can be about a few tens of milliseconds in some systems. Setting a shorter time than the fork cost means that the subshells are immediately killed before it does the actual task. So, I want to set the default value to be a relatively larger value, i.e., a longer time than the fork cost of arbitrary systems. (Actually, in some systems with antivirus software, the fork cost can be about 200 msec, but I think setting the timeout to be that order is noticeably unresponsive, so I chose 50 msec as a moderate value which is likely to be longer than most cases.)

Also, I'm wondering if the difference between 50 msec and 20 msec really matters. One may notice the difference of this 30 msec if one carefully compares the behavior, but I don't know if it really makes things different in real use.

What are the benefits of even having this sync timeout though? If you manage to implement the async highlighting why not just use that?

If we don't perform sync highlighting for glob patterns, the highlighting of glob patterns will not be performed at all while I am typing but will be performed after I stop the typing, which I don't like. Anyway, now it is configurable, so you can turn off the sync highlighting by setting highlight_timeout_sync=0.

I notice that setting this timeout to zero makes typing after the globstar pattern much worse though...

Oh, OK. That may be a problem.

Did you experience the worsening before the second push to spike (the latest spike) or after the second push? In the second push, I actually added special treatment for the case of zero timeouts, but I didn't think it really changes the behavior. In particular, I didn't think setting timeout to zero (in the first push) was worse than a small timeout, but I may miss something.

If you could reproduce it, could you tell me the details?

Also after typing a globstar path and even after this has been correctly highlighted, asking for completion with on something typed afterwards gets really slow.

Does the tab completion get slower than master? Or, are you just asking the reason that it takes again a long timeout for completion after the pathname expansion was already performed for highlighting? In the latter case, the pathname expansions performed for highlighting and completion have actually many differences, so they need to be performed separately; the most obvious difference is that for the word ?.tx the highlighting attempts the pathname expansion against ?.tx, but the completion against ?.tx*.

These improvements are really good, but I'm wondering if it would be simpler to just ignore highlighting on glob patterns since they might not even be accurate and not really relevant.

Yeah, the fact it can be inaccurate might be a problem, but the highlighting on glob patterns is really relevant for my usage. The purpose for highlighting is not just eye candies for simple paths like *.sh, but the highlighting also plays the role for the input checking of long paths like result/output-param1_*-param2_*-step*.txt. The highlighting tells me that there is at least one file path matching with the pattern, by which I know the glob pattern is correctly input.

As you have pointed out, highlighting for multiple file paths can be inaccurate, but it is rather obvious that the highlighting cannot show the status of multiple files, so I thought it is an easy guess that the highlighting is just for one path from the glob pattern. In fact, the highlighting is performed based on the first path generated by the pattern.

And these more complex patterns take much longer than running ls with the same pattern, and typing something afterwards becomes terrible...

Is it on the master branch or the current spike branch? Now we have timeouts and user-input cancellation in spike branch, so I thought it can't be worse than the usual command execution like ls $some_complicated_glob_pattern which we need to explicitly cancel by SIGINT (C-c) or something. If there is still a case that can be terrible in the spike branch, would you mind describing the details?

So overall, I think the benefits of highlighting these expressions might not be worth it....

Maybe this is only for me, but in my case, I feel it very useful in my long use history of ble.sh.

Edit: In case you don't want highlighting glob patterns, you can optionally turn it off by setting both timeouts zero: bleopt highlight_timeout_{sync,async}=0.

Maybe just ignoring the globstar patterns on highlighting would be better overall...

Yeah, that is the idea that I initially thought. One problem is that it is not so trivial to detect the globstar ** in the pattern. For example, consider "$path"/$pattern/*.sh where $path and $pattern may or may not contain **. Only ** in $pattern should be detected as a globstar.

Also, I don't want to complicate the rule observable to users in a way hard to guess. For example, if we only perform the highlighting on the glob patterns without a globstar **, when a glob pattern with a globstar is not highlighted, a user will think the path doesn't exist unless the user knows the special rule on the globstar.

@3ximus
Copy link
Author

3ximus commented Jan 31, 2021

Hi,
Yeah, I should have given more information on this. But all the things I reported were on the latest version of spike

I tried to show all the problems I mentioned, comparing performance against just executing a command with the globstar. Then performing completions after the globstar pattern is processed and simply typing while the pattern is being run.

Peek 2021-01-31 13-58

These improvements are really good, but I'm wondering if it would be simpler to just ignore highlighting on glob patterns since they might not even be accurate and not really relevant.

Yeah, the fact it can be inaccurate might be a problem, but the highlighting on glob patterns is really relevant for my usage. The purpose for highlighting is not just eye candies for simple paths like *.sh, but the highlighting also plays the role for the input checking of long paths like result/output-param1_*-param2_*-step*.txt. The highlighting tells me that there is at least one file path matching with the pattern, by which I know the glob pattern is correctly input.

As you have pointed out, highlighting for multiple file paths can be inaccurate, but it is rather obvious that the highlighting cannot show the status of multiple files, so I thought it is an easy guess that the highlighting is just for one path from the glob pattern. In fact, the highlighting is performed based on the first path generated by the pattern.

Ok, yeah I get your point, I have fucked up before due to globstars not expanding to anything xD

Also, on the spike branch the shell crashed on me a couple of times. I believe all of them happened when I hit tab to ask for a completion. I don't know more details but if I notice a pattern I'll let you know.

Cheers.

@akinomyoga
Copy link
Owner

akinomyoga commented Feb 1, 2021

Nice!

ls time and highlighting time

Maybe I again miss your point, but I guess from the animated gif the problem to be the longer time of highlighting the glob pattern than the execution time of the ls command. Actually, this time difference is inevitable because ble.sh is implemented in a Bash script which is slower than C programs. As far as ble.sh is implemented in the script, what we can do is just trying to respond to user input quickly leaving the unhighlighted words.

Slow even with bleopt highlight_timeout_sync=0

Hmm, it doesn't reproduce with my computers. I think the user-input cancellation of the background highlighting causes some delay, but I'm not sure what specifically causes this delay. I want to measure the time of each primitive operation. What are the results of the following commands?

$ _ble_measure_base=0 ble-measure 'ble/util/is-stdin-ready'
$ sleep 5 & pid=$!; ble-measure 'kill -0 $pid'; kill $pid
$ sleep 5 & pid=$! _ble_measure_base=0 _ble_measure_threshold=0 ble-measure 'kill $pid'
$ sleep 5 & pid=$!; kill $pid; _ble_measure_base=0 _ble_measure_threshold=0 ble-measure 'wait $pid'
$ (sleep 5) & pid=$!; kill $pid; _ble_measure_base=0 _ble_measure_threshold=0 ble-measure 'wait $pid'

Slow TAB completion with globstar words

OK, I got it. This is caused when ble.sh translates the current command line content to a plainer form for programmable completions like bash-completion. I'll consider an option to suppress pathname expansions for the programmable completions.

Crash on TAB completion

Also, on the spike branch the shell crashed on me a couple of times. I believe all of them happened when I hit tab to ask for a completion. I don't know more details but if I notice a pattern I'll let you know.

Thank you for the report! Hmm, that's a problem. It didn't happen to me, but I'll take a look at related codes to guess what happens.

@3ximus
Copy link
Author

3ximus commented Feb 2, 2021

Yeah here you go:
print_002

Maybe I again miss your point, but I guess from the animated gif the problem to be the longer time of highlighting the glob pattern than the execution time of the ls command. Actually, this time difference is inevitable because ble.sh is implemented in a Bash script which is slower than C programs. As far as ble.sh is implemented in the script, what we can do is just trying to respond to user input quickly leaving the unhighlighted words.

Yeah but I thought that since bash also expands the arguments to the ls the execution time for coloring the globstar expression would also be similar

@akinomyoga
Copy link
Owner

Re: Slow even with bleopt highlight_timeout_sync=0

Yeah here you go:

Thank you for your measurements! Hmm, it seems there is no bottleneck in the tested items. As I haven't yet reproduced this delay, I think I'll ask you for more detailed profiling if I cannot reproduce it for several days. Maybe I'll try other UNIX systems if I have time.

Re: ls time and highlighting time

Yeah but I thought that since bash also expands the arguments to the ls the execution time for coloring the globstar expression would also be similar

Ah, you're right. I measured the time and found that the highlighting time is three times as long as the time of the command execution. Then, I found that the reason is that, for the glob pattern **/*tool*/*.sh, ble.sh tries to perform three pathname expansions **/, **/*tool*/, and **/*tool*/*.sh to highlight each path segment (i.e., directory) in the path [Note that ble.sh highlights directories in different colors based on its sticky bit and whether it's a symbolic link].

Re: Slow TAB completion with globstar words

For this one, I wrote in the previous reply

OK, I got it. This is caused when ble.sh translates the current command line content to a plainer form for programmable completions like bash-completion.

but it wasn't actually the true cause. It turned out that in my host the slow TAB completion (after the highlighting of globstar patterns finished) doesn't reproduce. I noticed my misunderstanding after I implemented a new timeout bleopt complete_timeout_compvar based on the above wrong guess, but now I'm not sure if it changes anything. (Maybe you have already noticed, but I have pushed new commits to spike for bleopt complete_timeout_compvar. I also changed the name bleopt complete_auto_timeout to bleopt complete_timeout_auto.)

So, ... this is another problem that I couldn't reproduce. Maybe I'm going to ask for detailed profiling for this one as well.

Re: Crash on TAB completion

I haven't yet reproduced this either. I looked into the code but don't have an idea now.

@3ximus
Copy link
Author

3ximus commented Feb 3, 2021

Re: Slow even with bleopt highlight_timeout_sync=0

Yeah here you go:

Thank you for your measurements! Hmm, it seems there is no bottleneck in the tested items. As I haven't yet reproduced this delay, I think I'll ask you for more detailed profiling if I cannot reproduce it for several days. Maybe I'll try other UNIX systems if I have time.

Maybe don't worry about this, at least for now. It might have been an issue on my side since I noticed it doesn't reproduce every time. it could have been my HDD that is getting dubious read speeds xD

Re: ls time and highlighting time

Yeah but I thought that since bash also expands the arguments to the ls the execution time for coloring the globstar expression would also be similar

Ah, you're right. I measured the time and found that the highlighting time is three times as long as the time of the command execution. Then, I found that the reason is that, for the glob pattern **/*tool*/*.sh, ble.sh tries to perform three pathname expansions **/, **/*tool*/, and **/*tool*/*.sh to highlight each path segment (i.e., directory) in the path [Note that ble.sh highlights directories in different colors based on its sticky bit and whether it's a symbolic link].

Ok good, so this time can be shortened? Like the full glob can be expanded once, pick one of the results and just syntax highlight the glob according to that result? Ideally we could stop bash on the first expansion result and this would be nearly instant, but sadly we can't as you've noted before...

Re: Slow TAB completion with globstar words

For this one, I wrote in the previous reply

OK, I got it. This is caused when ble.sh translates the current command line content to a plainer form for programmable completions like bash-completion.

but it wasn't actually the true cause. It turned out that in my host the slow TAB completion (after the highlighting of globstar patterns finished) doesn't reproduce. I noticed my misunderstanding after I implemented a new timeout bleopt complete_timeout_compvar based on the above wrong guess, but now I'm not sure if it changes anything. (Maybe you have already noticed, but I have pushed new commits to spike for bleopt complete_timeout_compvar. I also changed the name bleopt complete_auto_timeout to bleopt complete_timeout_auto.)

So, ... this is another problem that I couldn't reproduce. Maybe I'm going to ask for detailed profiling for this one as well.

I just tried the new commits and it does improve TAB completion after globstar patterns. It's still not as smooth as without the pattern... Also I noticed something else that I'll put bellow.

Re: Crash on TAB completion

I haven't yet reproduced this either. I looked into the code but don't have an idea now.

Well, this didn't happen to me again, since then (I think I haven't used it that extensively these last few days either), however as I was typing this answer and about to record the gif bellow it just crashed when I was about to type ls ** before the first star. I'll inspect better the next time this happens...


For the new issues I found:

Peek 2021-01-31 13-58

Glob patterns don't work with paths starting with ~

And it seems that the speed of typing/tab completion is linked with the amount of expanded items in the same command, since it drastically increases cpu usage. Do you have idea what's happening? It shouldn't care for the expression that has already been calculated...

@akinomyoga
Copy link
Owner

akinomyoga commented Feb 4, 2021

Thank you for the continuing discussions!

Re: ls time and highlighting time

[...] Then, I found that the reason is that, for the glob pattern **/*tool*/*.sh, ble.sh tries to perform three pathname expansions **/, **/*tool*/, and **/*tool*/*.sh to highlight each path segment (i.e., directory) in the path [...]

Ok good, so this time can be shortened? Like the full glob can be expanded once, pick one of the results and just syntax highlight the glob according to that result?

Yeah, I thought a similar thing.

Would you be interested in contributing to this one? The above mentioned expansions are made in a single function ble/syntax:bash/simple-word/evaluate-path-spec. The function is not too complex, the goal of the improvement is clear, the necessary changes would probably be mainly in that function, so I think this improvement is suited to a so-called "Good First Issue" for ble.sh codebase. If you are interested, I will show the test case and goal and describe the current implementation, and then we can discuss possible improvements.

Re: Slow even with bleopt highlight_timeout_sync=0

Maybe don't worry about this, at least for now. It might have been an issue on my side since I noticed it doesn't reproduce every time. it could have been my HDD that is getting dubious read speeds xD

Maybe I was misunderstanding the speed level that you discuss. I somehow assumed there are severer delays in inputting texts, so I was trying to guess what causes a long delay. But now I came up with the possibility that you are rather focusing on the input experience. In that case, I had some improvement ideas (which I had kept from until the severe delay would be solved). Now I've implemented them in spike (3987c44):

  • Previously the timeout is set to each pathname expansions, which means that if there are three pathname expansions in highlighting, the total weight the user experiences is three times the timeout value set by highlight_timeout_{sync,async}. Now I changed the behavior so that the timeout propagates to later pathname expansions, i.e., when one pathname expansion times out, the following pathname expansions in the same highlighting process time out immediately.
  • I also added an option to change the interval of checking user inputs while the pathname expansions: bleopt syntax_eval_polling_interval=50. Also, I have changed the default value from 100 to 50 (The unit is a millisecond).

Could you see if anything changes with this commit (i.e., by comparing before and after the commit)?

Re: Crash on TAB completion

[...] it just crashed when I was about to type ls ** before the first star

May I confirm that it means the shell crashed with just "ls "? Maybe it is unrelated to pathname expansions, but there is another possibility that the glob patterns inserted by auto-complete caused a problem. If you started to see the crashes after switching to spike, I think the latter is likely.

By the way, I encountered hangs a few times while typing glob patterns in my Linux. I'm not sure if it is related to this crash, but I'll later fix it anyway.

Tilde prevents correct pathname expansions

Glob patterns don't work with paths starting with ~

Thank you for the report! It was a trivial mistake. I fixed it in spike (cc4df0b).

Speed is linked with the number of expanded items

And it seems that the speed of typing/tab completion is linked with the amount of expanded items in the same command, since it drastically increases cpu usage. Do you have idea what's happening? It shouldn't care for the expression that has already been calculated...

Maybe it is related to the cache size of pathname expansions. In ble.sh, even after the highlighting of the word is already calculated, there are chances that the expansion results are internally requested. In spike, the results of pathname expansions are cached, and when the pathname expansions for the same word are requested, these cached results are just returned. In this sense, pathname expansions are skipped. But if the amount of the expanded result is large, just returning the cache can still take time.

To check if that's the case or not, I added a commit for the experiment (640f32d) that restricts the number of words in a cache to two. Could you see if anything changes before and after this commit?

Thank you!

@3ximus
Copy link
Author

3ximus commented Feb 4, 2021

Re: ls time and highlighting time

Yeah, I thought a similar thing.

Would you be interested in contributing to this one? The above mentioned expansions are made in a single function ble/syntax:bash/simple-word/evaluate-path-spec. The function is not too complex, the goal of the improvement is clear, the necessary changes would probably be mainly in that function, so I think this improvement is suited to a so-called "Good First Issue" for ble.sh codebase. If you are interested, I will show the test case and goal and describe the current implementation, and then we can discuss possible improvements.

Yeah ok, I can give it a go 👍

Re: Slow even with bleopt highlight_timeout_sync=0

Could you see if anything changes with this commit (i.e., by comparing before and after the commit)?

Yeah, this commit with bleopt highlight_timeout_sync=0 makes typing after the glob much more fluid without any delays !

Re: Crash on TAB completion

By the way, I encountered hangs a few times while typing glob patterns in my Linux. I'm not sure if it is related to this crash, but I'll later fix it anyway.

I think I might be leading yo the wrong direction. The shell doesn't exactly crash, it just hangs there unresponsive... So it can be this problem that you mentioned. I noticed today that when it hangs it has only one subshell running ( as oposed to 2 when typing a glob pattern) and if I kill this subshell the shell works again. It did happen twice today, once when typing the first asterisk in a glob and a second when hitting tab on a normal path after I had a glob in the same command like: ls **/*.png ~/Documents/do<TAB>

Tilde prevents correct pathname expansions

Thank you for the report! It was a trivial mistake. I fixed it in spike (cc4df0b).

👍

Speed is linked with the number of expanded items

To check if that's the case or not, I added a commit for the experiment (640f32d) that restricts the number of words in a cache to two. Could you see if anything changes before and after this commit?

Awesome, this makes a huge difference! Everything seems to be working really well, including Tab completions after the glob pattern

@akinomyoga
Copy link
Owner

akinomyoga commented Feb 4, 2021

Re: ls time and highlighting time

Would you be interested in contributing to this one? [...] If you are interested, I will show the test case and goal and describe the current implementation, and then we can discuss possible improvements.

Yeah ok, I can give it a go 👍

OK. The target is the function ble/syntax:bash/simple-word/evaluate-path-spec defined in lib/core-syntax.sh. There are changes made in spike branch, so we want to modify the spike version of this function.

This function is used inside the highlighting something like in the following way:

ble/syntax:bash/simple-word/evaluate-path-spec '**/*tool*/*.sh' / after-sep:cached:stopcheck:timeout=50:timeout-carry

For the present purpose, the following command is enough to check the performance.

# Pre-condition: Ensure there is no timeout carry set by "timeout-carry" option
unset _ble_syntax_bash_simple_eval_timeout_carry

time ble/syntax:bash/simple-word/evaluate-path-spec '**/*tool*/*.sh' / after-sep

# Result
declare -p path spec ret
  • If you have executed the above version with timeout-carry option and caused timeouts, please unset _ble_syntax_bash_simple_eval_timeout_carry to clear the timeout carry flag, or otherwise it forcibly times out immediately.
  • The function writes the results to shell variables spec, path and ret. spec is an array containing e.g. spec=('**/' '**/*tool*/' '**/*tool*/*.sh'). The array path is the corresponding expansion result (with the first generated path for each element of spec). The array ret contains the full generated paths for the entire pattern **/*tool*/*.sh.

Currently, in my host, it causes three times as longer time as the single pathname expansion.

$ time ble/syntax:bash/simple-word/evaluate-path-spec '**/*tool*/*.sh' / after-sep

real    0m9.555s
user    0m4.544s
sys     0m4.853s
$ time ble/syntax:bash/simple-word/eval '**/*tool*/*.sh'

real    0m2.885s
user    0m1.179s
sys     0m1.658s

So, the goal is to shorten this time. Maybe it is not convenient to regenerate and reload ble.sh everytime you edit the function, so I think you can copy and rename the function in a file (e.g. evaluate-path-spec-v2.sh):

# evaluate-path-spec-v2.sh
function evaluate-path-spec-v2 {
  local word=$1 sep=${2:-'/:='} opts=$3
  ret=() spec=() path=()
  [[ $word ]] || return 0

  # read options
  local eval_opts=$opts notilde=
  [[ :$opts: == *:notilde:* ]] && notilde=\'\'

  # compose regular expressions
  local rex_element; ble/syntax:bash/simple-word/.get-rex_element "$sep"
  local rex='^['$sep']?'$rex_element'|^['$sep']'
  [[ :$opts: == *:after-sep:* ]] &&
    local rex='^'$rex_element'['$sep']?|^['$sep']'

  local tail=$word s= p= ext=0
  while [[ $tail =~ $rex ]]; do
    local rematch=$BASH_REMATCH
    s=$s$rematch
    ble/syntax:bash/simple-word/eval "$notilde$s" "$eval_opts"; ext=$?
    ((ext==148||ext==142)) && return "$ext"
    p=$ret
    tail=${tail:${#rematch}}
    ble/array#push spec "$s"
    ble/array#push path "$p"
  done
  [[ $tail ]] && return 1
  ((ext)) && return "$ext"
  return 0
}

# tests
unset _ble_syntax_bash_simple_eval_timeout_carry
time evaluate-path-spec-v2 '**/*tool*/*.sh' / after-sep
declare -p spec path

so that you can edit the file and measure the time by source evaluate-path-spec-v2.sh.

Now I think you can explore the implementation of this function and related codes by yourself, so I skip line-by-line explanations, but please feel free to ask about details if you have any questions.

  • ble/syntax:bash/simple-word/.get-rex_element "$sep" is used to compose a regular expression that is used to extract a path segment separated by one of the character in "$sep".
  • ble/array#push arr value is just equivalent to arr+=(value) in Bash 4.0+. There are some bugs in Bash 3.* with arr+=(value).
  • ble/syntax:bash/simple-word/eval causes a single pathname expansion. The exit status of this function is 148 when interrupted by user input. 142 when it times out. 1 when it fails with failglob, indirect parameter expansion, or other reasons. 0 with a successful expansion.

Re: Slow even with bleopt highlight_timeout_sync=0

Could you see if anything changes with this commit (i.e., by comparing before and after the commit)?

Yeah, this commit with bleopt highlight_timeout_sync=0 makes typing after the glob much more fluid without any delays !

OK, nice! I think I'm going to adjust this commit more.

Re: Crash on TAB completion

By the way, I encountered hangs a few times while typing glob patterns in my Linux. I'm not sure if it is related to this crash, but I'll later fix it anyway.

The shell doesn't exactly crash, it just hangs there unresponsive... So it can be this problem that you mentioned. I noticed today that when it hangs it has only one subshell running ( as oposed to 2 when typing a glob pattern) and if I kill this subshell the shell works again.

Great! This is actually what I observed in my host!

It did happen twice today, once when typing the first asterisk in a glob and a second when hitting tab on a normal path after I had a glob in the same command like: ls **/*.png ~/Documents/do<TAB>

Thank you! I still don't get the exact condition to reproduce it, so the information is helpful!

Re: Tilde prevents correct pathname expansions

Thank you for the report! It was a trivial mistake. I fixed it in spike (cc4df0b).

👍

Thank you!

Re: Speed is linked with the number of expanded items

To check if that's the case or not, I added a commit for the experiment (640f32d) that restricts the number of words in a cache to two. Could you see if anything changes before and after this commit?

Awesome, this makes a huge difference! Everything seems to be working really well, including Tab completions after the glob pattern

OK! I think the slow TAB completion even with the finished highlighting is also related to this one. The current commit is just for an experiment, and it breaks some functionality of ble.sh. I'll properly update the related codes based on this information.

@akinomyoga
Copy link
Owner

I have force-pushed to spike for adjustments of last commits.

Re: Slow even with bleopt highlight_timeout_sync=0

Yeah, this commit with bleopt highlight_timeout_sync=0 makes typing after the glob much more fluid without any delays !

OK, nice! I think I'm going to adjust this commit more.

I modified this one as the timeout carry only occurs in the sync highlighting (fd3b0c4). If this additional change has worsened the response, please let me know.

Re: Speed is linked with the number of expanded items

Awesome, this makes a huge difference! Everything seems to be working really well, including Tab completions after the glob pattern

OK! I think the slow TAB completion even with the finished highlighting is also related to this one. The current commit is just for an experiment, and it breaks some functionality of ble.sh. I'll properly update the related codes based on this information.

I've updated the code so that the caches for the first word and all the words are separately saved (a5f22cc). I guess this also solves or improves the problem "Re: Slow TAB completion with globstar words".

@akinomyoga
Copy link
Owner

Re: Crash/hang on TAB completion

These few days, I have worked on this problem. It seems to take more time to settle it, so I summarize the current status here. This seems like a problem of the implementation of read -t (read with a timeout which ble.sh uses for sleep&timeouts) in Bash.

The following script is a reproducer for the Bash problem. If you are interested, you can just run it as a normal Bash script to reproduce the problem. It should count up to 2000 without stopping. But, in Bash 4.3+, it stops randomly; read -t 0.001 fails to time out and block forever at a small probability. When it stopped, you can continue to the next number by killing the subshell by pressing C-c.

#!/bin/bash

rm -f a.pipe
mkfifo a.pipe
exec 9<> a.pipe
rm -f a.pipe
for c in {0..2000}; do
  (eval "echo {0..$c}" & read -u 9 -t 0.001) >/dev/null
  printf $'\r\e[Kok %d' "$c"
done
echo

Actually, it seems that read -t has always a non-zero probability to fail, but the probability largely depends on the combination with other Bash constructs. The combination (COMMAND-WRITING-TO-STDOUT & read -t TIMEOUT < PIPE/SOCKET), which I frequently used in the spike branch, causes a large probability of failing timeouts. The failing probability also depends on the system, the version of Bash, the release status (release, rc, beta, alpha or maint) of Bash build, the timeout duration. It also depends on the amount of output from the echo command in the above reproducer; there is a certain range of numbers (depending on the conditions) where the timeout failure frequently occurs. In the above reproducer, I have used a named pipe, but it also occurs with other pipes like the process substitutions and also with sockets like /dev/udp/0.0.0.0/80.

I have tested in Linux, Cygwin, FreeBSD, Minix with different versions of Bash. In Linux, Cygwin, and FreeBSD, the behavior is consistently reproduced with all the Bash versions whose version is 4.3 or higher including devel branch of Bash. The behavior has never occurred with older Bash versions. I couldn't reproduce it with Minix (Bash 4.3.0), but the stopping is random, so the probability is probably just small in Minix. Conversely, the probability is largest in Cygwin. Actually, I have been experiencing a few times a similar problem at a very small probability in Cygwin. Also, Bash compiled with maint mode (debugging mode) has a significantly smaller (but still non-zero) probability of the timeout failure compared to the release compile.

Here the results for different versions in different systems
  • Host 1: Linux
    MACHTYPE: x86_64-unknown-linux-gnu (Fedora release 30 (Thirty))
    CPU: Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz

    • OK - GNU bash, 4.0.44(2)-release (x86_64-unknown-linux-gnu)
    • OK - GNU bash, 4.1.17(2)-release (x86_64-unknown-linux-gnu)
    • OK - GNU bash, 4.2.53(2)-release (x86_64-unknown-linux-gnu)
    • Fail - GNU bash, 4.3.48(1)-release (x86_64-unknown-linux-gnu)
    • Fail - GNU bash, 4.4.23(1)-release (x86_64-unknown-linux-gnu)
    • Fail - GNU bash, 5.0.18(1)-release (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.0.7(1)-release (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.0.16(1)-release (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.0.18(1)-release (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.1.0(1)-alpha (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.1.0(1)-release (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.1.4(2)-maint (x86_64-pc-linux-gnu) (Current devel fcad1d1)
  • Host 2: Linux
    CPU: Intel(R) Core(TM) i5-8500 CPU @ 3.00GHz
    MACHTYPE: x86_64-unknown-linux-gnu (Fedora release 30 (Thirty))

    • OK - GNU bash, 4.0.44(2)-release (x86_64-unknown-linux-gnu)
    • OK - GNU bash, 4.1.17(2)-release (x86_64-unknown-linux-gnu)
    • OK - GNU bash, 4.2.53(2)-release (x86_64-unknown-linux-gnu)
    • Fail - GNU bash, 4.3.48(1)-release (x86_64-unknown-linux-gnu)
    • Fail - GNU bash, 4.4.23(1)-release (x86_64-unknown-linux-gnu)
    • Fail - GNU bash, 5.0.7(1)-release (x86_64-redhat-linux-gnu)
    • Fail - GNU bash, 5.0.18(1)-release (x86_64-pc-linux-gnu)
    • Fail - GNU bash, 5.1.0(1)-release (x86_64-pc-linux-gnu)
  • Host 3: Windows (Cygwin and also Arch, FreeBSD, Minix, and Solaris on VirtualBox)
    CPU: Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
    MACHTYPE: x86_64-unknown-cygwin (Cygwin)

    • Fail - GNU bash, 4.4.12(3)-release (x86_64-unknown-cygwin)

    MACHTYPE: x86_64-pc-linux-gnu (Arch Linux on VirtualBox)

    • Fail - GNU bash, 5.1.4(1)-release (x86_64-pc-linux-gnu)

    MACHTYPE: i386-portbld-freebsd12.0 (FreeBSD 12.0 on VirtualBox)

    • Fail - GNU bash, 5.0.11(0)-release (i386-portbld-freebsd12.0)

    MACHTYPE: i386-elf32-minix (Minix)
    Note: exec 9< <(sleep 60)

    • OK - GNU bash, 4.3.0(1)-release (i386-elf32-minix)

    MACHTYPE: i386-pc-solaris2.11 (Solaris on VirtualBox)

    • OK - GNU bash, 4.1.17(1)-release (i386-pc-solaris2.11)

I've performed git bisect against Bash devel branch and identified that the failure started to occur by the commit 10e7843 at bash. I extracted the minimal set of change that causes the failure as in the following "Critical change in Bash". This change was introduced after the discussion at bug-bash/2013-02/msg00016

Critical Bash change in 2013
10e784337238e6081ca5f9bdc8c21492f7b89388 is the first bad commit
commit 10e784337238e6081ca5f9bdc8c21492f7b89388
Author: Chet Ramey <chet@caleb.ins.cwru.edu>
Date:   Mon Mar 4 08:10:00 2013 -0500

    commit bash-20130208 snapshot

diff --git a/CWRU/CWRU.chlog b/CWRU/CWRU.chlog
index 04c629e7..59759f24 100644
--- a/CWRU/CWRU.chlog
+++ b/CWRU/CWRU.chlog
@@ -4502,3 +4502,23 @@ variables.c
 	  but create them as (invisible) exported variables so they pass
 	  through the environment.  Print an error message so user knows
 	  what's wrong.  Fixes bug reported by Tomas Trnka <ttrnka@mail.muni.cz>
+
+				    2/9
+				    ---
+
+builtins/read.def
+	- sigalrm_seen, alrmbuf: now global so the rest of the shell (trap.c)
+	  can use them
+	- sigalrm: just sets flag, no longer longjmps to alrmbuf; problem was
+	  longjmp without manipulating signal mask, leaving SIGALRM blocked
+
+quit.h
+	- move CHECK_ALRM macro here from builtins/read.def so trap.c:
+	  check_signals() can call it
+
+trap.c
+	- check_signals: add call to CHECK_ALRM before QUIT
+	- check_signals_and_traps: call check_signals() instead of including
+	  CHECK_ALRM and QUIT inline.  Integrating check for read builtin's
+	  SIGALRM (where zread call to check_signals_and_traps can see it)
+	  fixes problem reported by Mike Frysinger <vapier@gentoo.org>
diff --git a/builtins/read.def b/builtins/read.def
index ea2667bb..7a1dff6f 100644
--- a/builtins/read.def
+++ b/builtins/read.def
@@ -136,27 +136,21 @@ static void ttyrestore __P((struct ttsave *));
 static sighandler sigalrm __P((int));
 static void reset_alarm __P((void));
 
-static procenv_t alrmbuf;
-static int sigalrm_seen, reading;
+procenv_t alrmbuf;
+int sigalrm_seen;
+static int reading;
 static SigHandler *old_alrm;
 static unsigned char delim;
 
 /* In most cases, SIGALRM just sets a flag that we check periodically.  This
    avoids problems with the semi-tricky stuff we do with the xfree of
    input_string at the top of the unwind-protect list (see below). */
-#define CHECK_ALRM \
-  do { \
-    if (sigalrm_seen) \
-      longjmp (alrmbuf, 1); \
-  } while (0)
 
 static sighandler
 sigalrm (s)
      int s;
 {
   sigalrm_seen = 1;
-  if (reading)		/* do the longjmp if we get SIGALRM while in read() */
-    longjmp (alrmbuf, 1);
 }
 
 static void
diff --git a/quit.h b/quit.h
index 8df01e1f..7d447ab2 100644
--- a/quit.h
+++ b/quit.h
@@ -36,6 +36,11 @@ extern volatile int terminating_signal;
     if (interrupt_state) throw_to_top_level (); \
   } while (0)
 
+#define CHECK_ALRM \
+  do { \
+    if (sigalrm_seen) \
+      longjmp (alrmbuf, 1); \
+  } while (0)
 #define SETINTERRUPT interrupt_state = 1
 #define CLRINTERRUPT interrupt_state = 0
 
diff --git a/trap.c b/trap.c
index d60a7fb4..b030747b 100644
--- a/trap.c
+++ b/trap.c
@@ -88,6 +88,8 @@ static void reset_or_restore_signal_handlers __P((sh_resetsig_func_t *));
 extern int last_command_exit_value;
 extern int line_number;
 
+extern int sigalrm_seen;
+extern procenv_t alrmbuf;
 extern char *this_command_name;
 extern sh_builtin_func_t *this_shell_builtin;
 extern procenv_t wait_intr_buf;
@@ -456,18 +459,20 @@ any_signals_trapped ()
   return -1;
 }
 
 /* Convenience functions the rest of the shell can use */
 void
 check_signals_and_traps ()
 {
+  check_signals ();
   QUIT;
   run_pending_traps ();
 }
 
 void
 check_signals ()
 {
+  CHECK_ALRM;		/* set by the read builtin */
   QUIT;
 }
 
 #if defined (JOB_CONTROL) && defined (SIGCHLD)

I got a stacktrace when read -t is blocked (with modified Bash based on 10e7843):

#0  0x00007f71518f4145 in read () from /lib64/libc.so.6
#1  0x000000000048f665 in zread (fd=9, buf=0x7fff0fd32c3a "", len=1) at zread.c:51
#2  0x000000000047f995 in read_builtin (list=0x0) at ./read.def:513
[...]

When read -t correctly times out, the stacktrace is like this:

#1  check_signals () at trap.c:475
#2  0x000000000045b9ab in check_signals_and_traps () at trap.c:465
#3  0x000000000048f667 in zread (fd=9, buf=0x7fffffffcdfa "", len=1) at zread.c:52
#4  0x000000000047f9a5 in read_builtin (list=0x0) at ./read.def:516
[...]

So, before the change, Bash cancels read by longjmp in the signal handler for SIGALRM. However, after the change, Bash just flags the variable sigalrt_seen in the signal handler and waits for the read failure with EINTR (caused by SIGALRT). The problem of this newer approach is that if SIGALRM arrives when Bash is not waiting for a file descriptor in read(2), it loses the chance to get EINTR from read and would be blocked forever.

After the change in 2013, a related change has been made in 2018 as follows. This change was introduced after the discussion bug-bash/2018-01/msg00114. It seems that the problem actually has been already reported, but only "a partial fix" has been made at that time. Chet added an additional check on sigalrm_seen just before the call of read(2), but this cannot prevent SIGALRM between the check and the call of read(2) as Chet correctly mentioned in bug-bash/2018-01/msg00128.

Bash change in 2018
                   1/31
                   ----
lib/sh/zread.c
    - zread,zreadintr: call check_signals() before calling read() to
      minimize the race window between signal delivery, signal handling,
      and a blocking read(2). Partial fix for FIFO read issue reported by
      Oyvind Hvidsten <oyvind.hvidsten@dhampir.no>

diff --git a/lib/sh/zread.c b/lib/sh/zread.c
index 14b66381..8b7ecedf 100644
--- a/lib/sh/zread.c
+++ b/lib/sh/zread.c
@@ -53,6 +53,7 @@ zread (fd, buf, len)
 {
   ssize_t r;

+  check_signals ();^I/* check for signals before a blocking read */
   while ((r = read (fd, buf, len)) < 0 && errno == EINTR)
     /* XXX - bash-5.0 */
     /* We check executing_builtin and run traps here for backwards compatibility */

I've tested for this fix, but it seems this fix actually didn't have much effect on the above reproducer. I also investigated the timing when SIGALRM arrives, but it seems SIGALRM happens inside read(2) even in the case of the timeout failure. Also, the timeout failure typically occurs with FIFO and sockets, which means that it takes some time for read(2) to be ready to handle signals for special types of file descriptors.

In this case, I don't know what is the proper way to fix Bash. Maybe we should use select(2) and non-blocking read(2)? Maybe we should revert the change in 2013 and take another approach to the problem reported in 2013? Anyway, I think I'll report this one on the bug-bash mailing list later.

The problem is present in Bash 4.3, 4.4, 5.0, and 5.1 in any way. I think from the experience, even if this is fixed in devel Bash, Chet won't apply the patch to these existing releases. Therefore we need to think about how we can work around this broken read timeout at the ble.sh side.

@akinomyoga
Copy link
Owner

akinomyoga commented Feb 9, 2021

Re: Crash/hang on TAB completion

I pushed a workaround for this in spike (57ce48b I have force-pushed f17d933). This workaround is not a perfect one; there is still a non-zero probability of blocking in theory, but I believe this significantly reduces the blocking probability. At least I didn't observe any blocking with this workaround after 200 trials of the test case in the previous comment (200x2000 = 400000 read -t).

I also reported the issue to the bug-bash mailing list with a patch:
https://lists.gnu.org/archive/html/bug-bash/2021-02/msg00059.html

Can you test the latest version of spike for a while to see if the hanging has disappeared? Also, I would like to hear the recent situation on other problems ("Slow even with bleopt highlight_timeout_sync=0", "Slow TAB completion with globstar highlighted already", and "Speed is linked with the number of expanded items"). There seem to be no critical bugs other than this, so I'm thinking of pushing these commits to master after several days.

How's it going with the improvement on evaluate-path-spec? Because the codebase of ble.sh is not so organized, I think it might take a long time to understand it and edit the code with confidence if you try to do everything by yourself. Please feel free to ask any questions :)

Thank you!

@3ximus
Copy link
Author

3ximus commented Feb 13, 2021

Hi, I'm sorry for not replying sooner, I haven't had the time to check this out and I'm sorry if I'm delaying things for you.

Re: ls time and highlighting time

Thank you for your detailed explanation, as soon as I find some time I'll get into it. But if you don't want to wait for me it's ok, you can probably fix it quite easily

Re: Crash/hang on TAB completion

Awesome that you found all this and that you might be able to fix it, I'm gonna update to the latest version, use for a few days and let you know

@akinomyoga
Copy link
Owner

Thank you for your reply!

Re: ls time and highlighting time

It's not in a rush so please take your time. I usually unset the globstar option, and the improvement of evaluate-path-spec doesn't affect other parts of the codebase so that it can be relatively independently edited.

Re: Crash/hang on TAB completion

Thank you! I look forward to seeing your feedback! Hope everything works fine.

@akinomyoga
Copy link
Owner

akinomyoga commented Feb 20, 2021

I have pushed spike commits to the master branch. It seems there are no additional performance problems, I've checked miscellaneous existing performance issues as solved (except for the improvement on evaluate-path-spec). If there is still some performance issue or delay, please let me know! Thank you!

Edit: I have deleted the spike branch, so please switch back to the master branch hereafter.

@3ximus
Copy link
Author

3ximus commented May 4, 2021

Hi there, sorry for not saying anything anymore. I haven't had time/energy to check this and do the changes we mentioned.
I think you can close this issue now since performance for globstar is quite good imo
Cheers 👍

@akinomyoga
Copy link
Owner

OK! Thank you for letting me know that. I'll close this issue after I implement the suggested optimization for ble/syntax:bash/simple-word/evaluate-path-spec. But its priority is not so high now, so maybe it will be some time later.

@3ximus
Copy link
Author

3ximus commented May 4, 2021

Yeah, no problem 👍

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

No branches or pull requests

2 participants