complete: simple performance improvements (see #64) #65
Conversation
There was a problem hiding this comment.
Thank you! Nice! I have a few discussions.
complete_limit is checked only in some cases, namely those where I was facing prohibitive processing time: I didn't want to add checks everywhere without testing them.
OK. Actually, that was what I intended: we can add checks anytime later when we noticed some performance issues.
My Bash skills are rather limited and this project is fairly complex so please don't hesitate to ask me to fix this PR if necessary.
Your code looks fine! It is not a matter of skill, but there are certain coding styles of ble.sh and also some bugs of old Bash. I will check for such trivial fixes after we settled the design for this new feature. For now, let us discuss the detailed behavior of the new option. I have several comments as will be written in the subsequent review comments.
blerc
I would appreciate it if you could add a description of the new option in blerc template.
substr pattern
Outdated. Please forget about this discussion.
I didn't implement a dedicated substr pattern for filenames because it would have required to significantly alter the substr filter and I wanted to keep this patch small.
I looked into the code, and now I see your point. The current substr filter is so general that it doesn't treat the path separator / in a special way. We might want some pattern like *dir1*/*dir2*/*file* for the path dir1/dir2/file , but the filter only accepts the pattern like *dir1/dir2/file*.
I thought of allowing the skip of filtering when the source itself does the filtering, but the current implementation in ble.sh is not so clean that it is possible. I think I later change to perform the filtering in ble/complete/cand/yield instead of after the call of sources, and then add the way to skip filtering.
For now, to match with the current behavior of Edit: I have applied such a change in 7c6b67b along with other cleanups of filtering codes.substr filter, could you please just generate a pattern as ble/string#quote-word "$path"; local pattern=*$ret* for *:m:*?
See updates in #65 (comment) .
Thank you again for your PR!
|
I've tried. I'm thinking that we can change the limit for tab-completion and auto-completion, i.e., the latter is smaller than the former. What do you think? For example, add diff --git a/lib/core-complete.sh b/lib/core-complete.sh
index ee90fde..ed10296 100644
--- a/lib/core-complete.sh
+++ b/lib/core-complete.sh
@@ -1312,6 +1312,14 @@ function ble/complete/action:variable/init-menu-item {
#------------------------------------------------------------------------------
# source
+function ble/complete/source/test-limit {
+ local value=$1 limit=$bleopt_complete_limit
+ [[ $bleopt_complete_limit_auto && :$comp_type: == *:auto:* ]] &&
+ limit=$bleopt_complete_limit_auto
+ ((limit=limit)) # evaluate arithmetic expression
+ ((limit<=0||value<=limit))
+}
+
## 関数 ble/complete/source/reduce-compv-for-ambiguous-match
## 曖昧補完の為に擬似的な COMPV と COMPS を生成・設定します。
## @var[in,out] COMPS COMPV |
|
Hi! Sorry, I'm a bit busy right now, but I will take care of this and rework the patch in the next couple of weeks! |
|
@timjrd OK, thank you for letting me know that! Please take your time. I have updates on If you are interested in the reasons, here is the detail.
|
timjrd
force-pushed
the
complete-limit
branch
from
a8d2193
to
64514be
Compare
|
Hi! Here is the patch reworked according to your comments. In the meantime I switched to infinite Bash history and after some time auto-completion became very slow, I thought I could add a limit check inside the history source, especially since other users are experiencing issues with big histories. I also faced huge slowdowns with completion on paths containing wildcards. I will try to address these two issues when I have some time, but I need to reproduce them reliably first. Maybe this would be a better fit for another PR. Concerning the visual bell, it's not a big deal, but do you think it would be possible to display the message Thanks! |
|
Nice! Thank you!
I think they can be discussed in separate PRs. Let's first settle this PR, and then you can make another PR/PRs.
May I ask in detail? In what sense it is slow?
I actually doubt if his problem is really related to the big history. As already written, the history-based completion is designed not to block the user input, i.e., the processing will be immediately canceled when there is user input, so I don't have an idea what happened there. Then he didn't respond after that, so there was no chance to get a hint on the problem. Usually, the initial guess of users (who didn't really look into the code) is wrong (or, in other words, mostly random) to my experience. In particular, a large history is a popular naive guess in the past issues actually. Re: Limit reached message
Yes, it is possible. Actually, I was also thinking a similar idea. You can use the function ble-edit/info/show ansi $'\e[38;5;242m(too many completions)\e[m'But, here we need to carefully design the behavior because the limit check is performed for each source which means that, even when some source reached the limit, another source might still generate candidates. In that case, both the message and the completions cannot be shown in the same place at the same time. For example, how about showing the message below the command line only when there are no completions at all, or otherwise ringing bells? |
|
Hi, thanks for your feedback!
I'm pretty sure that the slowdowns were caused by the Bash option
Sounds fine! |
OK! I think this causes the slowdown when a command is executed and thus added to the command history, but does it cause the slowdown to
OK!
Thank you! Then, you can switch to |
timjrd
force-pushed
the
complete-limit
branch
from
64514be
to
1a4f87e
Compare
|
Hi! Here is the patch reworked with exit codes. I tried to use |
|
Thank you! May I ask if you have diff between the previous patch and the current patch? Since the changes are squashed (or the commit is directly amended), it is cumbersome to track what has been updated since the previous patch. I would like to avoid reviewing the same code from the beginning many times. Of course, I can still create temporary two branches on a certain commit at my local repository, cherry-pick both the previous and current patches on each branch and perform Anyway this time I'll make Edit: Here is the diff.diff --git a/lib/core-complete.sh b/lib/core-complete.sh
index 32b032a..e9f311c 100644
--- a/lib/core-complete.sh
+++ b/lib/core-complete.sh
@@ -1297,23 +1297,21 @@ function ble/complete/action:variable/init-menu-item {
#------------------------------------------------------------------------------
# source
-## Takes an array by reference as $1 and empty it if its size exceeds
-## $bleopt_complete_limit or $bleopt_complete_limit_auto.
+## Test whether $1 exceeds $bleopt_complete_limit or
+## $bleopt_complete_limit_auto.
##
-## ble/complete/source/limit my_array # no '$'
-##
-function ble/complete/source/limit {
- declare -n ref=$1
-
+function ble/complete/source/test-limit {
if [[ :$comp_type: == *:auto:* && $bleopt_complete_limit_auto ]]; then
local limit=$bleopt_complete_limit_auto
else
local limit=$bleopt_complete_limit
fi
- if [[ $limit && ${#ref[@]} -gt $limit ]]; then
+ if [[ $limit && $1 -gt $limit ]]; then
complete_limit_reached=1
- ref=()
+ return 1
+ else
+ return 0
fi
}
@@ -1593,6 +1591,7 @@ function ble/complete/source:command/gen {
local ret
ble/complete/source:file/.construct-pathname-pattern "$COMPV"
ble/complete/util/eval-pathname-expansion "$ret/"
+ ble/complete/source/test-limit ${#ret[@]} || return 1
((${#ret[@]})) && printf '%s\n' "${ret[@]}"
fi
}
@@ -1626,7 +1625,7 @@ function ble/complete/source:command {
[[ $compgen ]] || return 1
ble/util/assign-array arr 'ble/bin/sort -u <<< "$compgen"' # 1 fork/exec
- ble/complete/source/limit arr
+ ble/complete/source/test-limit ${#arr[@]} || return 1
for cand in "${arr[@]}"; do
((cand_iloop++%bleopt_complete_polling_cycle==0)) && ble/complete/check-cancel && return 148
@@ -1703,8 +1702,6 @@ function ble/complete/util/eval-pathname-expansion {
ble/array#reverse dtor
ble/util/invoke-hook dtor
-
- ble/complete/source/limit ret
}
## 関数 ble/complete/source:file/.construct-ambiguous-pathname-pattern path
@@ -1777,6 +1774,8 @@ function ble/complete/source:file/.impl {
[[ :$opts: == *:directory:* ]] && ret=${ret%/}/
ble/complete/util/eval-pathname-expansion "$ret"
+ ble/complete/source/test-limit ${#ret[@]} || return 1
+
candidates=()
local cand
if [[ :$opts: == *:directory:* ]]; then
@@ -2396,7 +2395,7 @@ function ble/complete/progcomp/.compgen {
fi
} 2>/dev/null
- ble/complete/source/limit arr
+ ble/complete/source/test-limit ${#arr[@]} || return 1
local action=progcomp
[[ $comp_opts == *:filenames:* && $COMPV == */* ]] && COMP_PREFIX=${COMPV%/*}/
@@ -2883,6 +2882,7 @@ function ble/complete/source:argument {
local ret cand
ble/complete/source:file/.construct-pathname-pattern "$value"
ble/complete/util/eval-pathname-expansion "$ret"
+ ble/complete/source/test-limit ${#ret[@]} || return 1
for cand in "${ret[@]}"; do
[[ -e $cand || -h $cand ]] || continue
[[ $FIGNORE ]] && ! ble/complete/.fignore/filter "$cand" && continue
@@ -2909,7 +2909,7 @@ function ble/complete/source/compgen {
local arr
ble/util/assign-array arr 'builtin compgen -A "$compgen_action" -- "$compv_quoted"'
- ble/complete/source/limit arr
+ ble/complete/source/test-limit ${#arr[@]} || return 1
# 既に完全一致している場合は、より前の起点から補完させるために省略
[[ $1 != '=' && ${#arr[@]} == 1 && $arr == "$COMPV" ]] && return 0
@@ -6311,6 +6311,7 @@ function ble/cmdinfo/complete:cd/.impl {
local ret cand
ble/complete/source:file/.construct-pathname-pattern "$COMPV"
ble/complete/util/eval-pathname-expansion "$name$ret"
+ ble/complete/source/test-limit ${#ret[@]} || return 1
for cand in "${ret[@]}"; do
((cand_iloop++%bleopt_complete_polling_cycle==0)) &&
ble/complete/check-cancel && return 148
@@ -6339,6 +6340,7 @@ function ble/cmdinfo/complete:cd/.impl {
local ret cand
ble/complete/source:file/.construct-pathname-pattern "$COMPV"
ble/complete/util/eval-pathname-expansion "${ret%/}/"
+ ble/complete/source/test-limit ${#ret[@]} || return 1
for cand in "${ret[@]}"; do
((cand_iloop++%bleopt_complete_polling_cycle==0)) &&
ble/complete/check-cancel && return 148 |
For example, $ bleopt complete_limit=5 complete_ambiguous=
$ h[TAB]Now I tried to implement the original suggestion of showing messages in the next line, but the behavior seemed to be a bit unnatural, so now I think we don't have to support it. Here's the detail: Now I tried the suggested behavior with the following change, but it seems the behavior is a bit unnatural in vim mode. Actually, the line below the commandline is used for various purposes including the completion menu, current editing modes (various maps in vim / MULTILINE in emacs), error messages, etc. If the completion sets the error message, this message diff --git a/lib/core-complete.sh b/lib/core-complete.sh
index e9f311c..c7d8931 100644
--- a/lib/core-complete.sh
+++ b/lib/core-complete.sh
@@ -4804,17 +4804,21 @@ function ble/widget/complete {
ble/complete/generate-candidates-from-opts "$opts"; local ext=$?
if ((ext==148)); then
return 148
- fi
+ elif ((ext!=0||cand_count==0)); then
if [[ $complete_limit_reached ]]; then
+ ble-edit/info/show ansi $'\e[38;5;242m(too many completions)\e[m'
+ else
[[ :$opts: != *:no-bell:* ]] &&
- ble/widget/.bell 'complete: limit reached'
- fi
- if ((ext!=0||cand_count==0)); then
- [[ :$opts: != *:no-bell:* && -z $complete_limit_reached ]] &&
ble/widget/.bell 'complete: no completions'
ble-edit/info/clear
+ fi
return 1
fi
+
+ if [[ $complete_limit_reached ]]; then
+ [[ :$opts: != *:no-bell:* ]] &&
+ ble/widget/.bell 'complete: limit reached'
+ fi
fi
if [[ :$opts: == *:insert_common:* ]]; thenSo, now I think we don't have to support the special treatment using |
|
I think it's almost done! The remaining are just trivial fixes and cosmetic changes.
|
|
Hi! How are you doing? It's not in a rush, but do you have a plan to finalize this PR? If you have already lost interest in this PR, I'm ready to continue finalizing the PR at my side (yet it seems the remaining fixes are rather trivial ones) :) Or, if you are just busy recently, I would appreciate it if you could just tell me that you are busy. Again, It's not in a rush, so I can wait as far as you don't lose interest! Thank you! |
|
Hi! Sorry for the delay, I must admit I was a bit busy (and also a bit lazy 😅). I will finalise the PR this weekend!
Le 21 janvier 2021 20:54:42 GMT+01:00, Koichi Murase <notifications@github.com> a écrit :
…Hi! How are you doing? It's not in a rush, but do you have a plan to
finalize this PR? If you have already lost interest in this PR, I'm
ready to continue finalizing the PR at my side (yet it seems the
remaining fixes are rather trivial ones) :) Or, if you are just busy
recently, I would appreciate it if you could just tell me that you are
busy. Again, It's not in a rush, so I can wait as far as you don't lose
interest! Thank you!
|
|
OK! Thank you for your quick response! |
|
Hi :) Here are the remaining fixes. Thanks! |
|
Thank you! Looks fine! I merge it. |
|
Thank you so much for all the discussion and your work! I have merged the commit. I closed Issue #64 with this PR. If you think we need further discussion in #64 for another possible PR, please feel free to reopen it. I also added related descriptions to the manual ( If you are willing in the future to discuss another performance improvement that you have mentioned in the following comment, you are always welcome!
Thanks again for your work! |
|
I hope this tiny feature will prove useful :). Thanks for your time and for your help! |
|
Hi @timjrd. Recently, based on detailed reports in #82 by @3ximus, I have been adding a number of optimizations to completion and highlighting, which I think have some overlaps with your contributions. Today I pushed them to the Now I think the user experience became much more responsive than before, so I'm thinking of increasing the default limit values for |
|
Hi! Sorry for the delay. I just tested revision |
|
I tried to re-enable (unrelated, but this latest version doesn't work with the Kitty terminal emulator (seems to work fine with gnome-terminal though)) |
|
Thank you for the feedback!
Does it mean the shell doesn't respond soon when the user supplies additional inputs while TAB completion? If the user doesn't input anything while the TAB completion, |
OK, I could reproduce it. By the way, what is your operating system? I reproduced it with Linux (Fedora).
Thank you for the report. It seems there is a Linux version of Kitty, so I think I'll try it later. |
For my use case, that's mostly the same problem as before: when I attempt a tab-completion a my very large directory, Bash is maxing up one CPU core for 10+ seconds. When I want to type something, the completion is correctly interrupted, but sometimes it takes a few seconds to respond.
I'm using Linux too, with an ext4 filesystem, and Bash 5. I'm using the NixOS distribution which is why I have to deal with a huge directory (the Nix store, ~40k entries on my system). |
|
In the previous commit, I wrote I could reproduce it, but possibly it was a different issue... In my case, the problem has occurred in some loops inside an old version of May I ask questions to check whether your performance issue is related to the above one or not?
|
|
I pushed some fixes to In commit 856cec2, I fixed the issue I mentioned in the previous reply. To stop the loops (
In commit eca2976, I slightly changed an escape sequence so that Kitty can recognize it. Kitty doesn't recognize |
|
Hi! Thanks a lot for these fixes! The following tests were done on
Yes. Same thing.
Same thing with |
|
OK! Thank you for testing! There seems to be still the performance issue even with the latest version in your environment. Actually, now I cannot reproduce the problem in my environment. For example, it works smoothly with the following command line: $ mkdir {00000..19999}
$ touch {00000..19999}.txt
$ echo 012345
You have told me that it sometimes takes a few seconds (as quoted below), but can you tell roughly how often it happens?
Have you experienced the delay in more than one NixOS system? If it consistently reproduces in multiple NixOS hosts, maybe I can set up my own NixOS distribution in my VirtualBox. |
|
Did you try: Without any prefix? |
|
OK, thank you for the information. I could reproduce it with 100k files + 100k directories and a certain interval between |
|
I guess you have a NVMe SSD and a beefier CPU, or maybe XFS is doing a better job here xD |
|
I found one bottleneck, but there are also several suspicious places. The processing speed of these suspicious places are not so bad in my environment, but they might be slow in your environment. Could you provide me the result of the following commands in normal Bash (i.e., with $ cd /path/to/big-directory
$ time files=(*)
$ time def=$(declare -p files)
$ time def=ret=${def#*=}
$ time eval "$def"
$ echo ${#ret[@]}
$ time def="ret=(${files[*]@Q})" |
|
Here you go: |
|
@timjrd Thank you for your results, and sorry for the long blank. After various experiments, I finally added another performance improvement in c89aa23. Thanks to your measurement, the following part turned out to have a significant delay (which was actually not so slow in my system with 100k elements).
A similar operation has been used in Hope it could improve the situation in
|
This implements what was discussed in #64. I didn't implement a dedicated
substrpattern for filenames because it would have required to significantly alter thesubstrfilter and I wanted to keep this patch small.complete_limitis checked only in some cases, namely those where I was facing prohibitive processing time: I didn't want to add checks everywhere without testing them.My Bash skills are rather limited and this project is fairly complex so please don't hesitate to ask me to fix this PR if necessary. Also, maybe this needs more testing: I'm switching my
ble.shinstallation to this branch.Edit: Resolve #64