Event driven design #285
Event driven design #285
Conversation
|
I'm really excited for a feature such as this! I'm on macOS, which really might be the culprit, and I can't really seem to get this to run properly. Is there a difference between how file descriptors are on macOS and on OpenBSD (your OS of choice) that messes with this? |
Codecov Report
@@ Coverage Diff @@
## master #285 +/- ##
=========================================
- Coverage 90.28% 89.4% -0.88%
=========================================
Files 1 1
Lines 494 538 +44
=========================================
+ Hits 446 481 +35
- Misses 48 57 +9
Continue to review full report at Codecov.
|
|
Just as I commented I saw the update in 423c87d, and I have the same experience with that patch too. |
|
This is awesome and #22 can finally be closed 👍 ... but on some use cases the old method is faster: % find . -name "*" | pick -q "modcgi.mk"
... same with "poll": (the directory contains 74447 entries and is already cached ... the machine is old: 32 bit/3.06GHz) |
@teoljungberg bummer, I'm out of ideas on how-to solve this on macOS. |
@DBOTW try the latest commit on this branch. Does it improve the |
It does: ... but pasting a query with middle-click does not work if no key is pressed before ... |
Try the latest commit. |
|
It works only with this change: diff --git a/pick.c b/pick.c
index fd64bc5..d0b7d82 100644
--- a/pick.c
+++ b/pick.c
@@ -535,7 +535,7 @@ selected_choice(void)
cursor_position += length;
query_length += length;
query[query_length] = '\0';
- dofilter = 1;
+ dofilter = choices_reset = 1;
break;
case UNKNOWN:
break; |
@mptre That's sadly more than I can help you with. Maybe @calleerlandsson has a trick up his sleeve? |
@DBOTW try the latest commit please. |
|
@teoljungberg try running the following program, let i run for a second $ cc -x c -o tty - <<EOF && ./tty
#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
int
main(void)
{
ssize_t n;
int fd, flags;
unsigned char c;
fd = open("/dev/tty", O_RDONLY);
if (fd == -1)
err(1, "open");
flags = fcntl(fd, F_GETFL, 0);
if (flags == -1)
err(1, "fcntl: F_GETFL");
if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1)
err(1, "fcntl: F_SETFL");
for (;;) {
n = read(fd, &c, 1);
fprintf(stderr, "%s: n=%ld\n", __func__, n);
if (n == -1 && errno != EAGAIN)
err(1, "read");
if (n == 1)
break;
sleep(1);
}
close(fd);
fprintf(stderr, "%#x\n", c);
return 0;
}
EOFMy latest idea is as follows: while stdin has not yet reached EOF, poll |
|
@mptre That seems about right: |
|
@mptre Works perfect now! 👍 |
mptre
force-pushed
the
poll
branch
2 times, most recently
from
faef555 to
ab725e9
Compare
|
@teoljungberg please try the latest commit. |
|
@mptre It compiles! I did a crazy thing and tried But - since we are now tentatively handling more files. Are there performance gains we can gain somehow? |
|
It looks like the polling is still occurring after selection has been "selected". |
|
Tested latest commit: blazing fast! 🥇 diff --git a/pick.c b/pick.c
index 1af8abb..d32c589 100644
--- a/pick.c
+++ b/pick.c
@@ -526,6 +526,7 @@ selected_choice(void)
cursor_position += length;
query_length += length;
query[query_length] = '\0';
+ selection = yscroll = 0;
dofilter = 1;
break;
case UNKNOWN:
@@ -533,12 +534,14 @@ selected_choice(void)
}
render:
- if (choices_reset)
+ if (choices_reset) {
choices_count = choices.length;
+ selection = yscroll = 0;
+ }
choices_reset = 0;
if (dofilter) {
if ((dochoices = filter_choices(choices_count)))
- dofilter = selection = yscroll = 0;
+ dofilter = 0;
}
tty_putp(cursor_invisible, 0); |
Glad to hear!
Also tried to fix this. There's one problem with your diff: $ (echo a; echo aa;) | pick) # type: a DOWN a... the selection is now out of bounds. |
@teoljungberg not sure I follow, does it leave your terminal in a weird state? |
|
@mptre it does, and only if I select something while the list to pick is still being populated from the stream. Worth noting, this still happens with tools such as |
|
Now that I compared it to the speed of |
|
Just pushed another performance improvement. Here's some number of the actual $ find /usr/src -type f | wc -l
85078
$ find /usr/src -type f | env LINES=10 ./pick -Xq realloc
master branch: 2.3s
poll branch: 8.0s
poll branch with latest improvement: 2.4sThe performance looks similar to the current version of pick. This is not the It would be interesting if anyone else could perform similar benchmarks using a polldiff --git a/pick.c b/pick.c
index f128349..5fe50e1 100644
--- a/pick.c
+++ b/pick.c
@@ -335,6 +335,9 @@ selected_choice(void)
int dochoices = 0;
int dofilter = 1;
+ struct timespec t0, t1, t2;
+ clock_gettime(CLOCK_MONOTONIC, &t0);
+
cursor_position = query_length;
/* No timeout on first call to fdwait in order to render the UI fast. */
@@ -370,6 +373,9 @@ selected_choice(void)
}
} else {
fd = -1; /* EOF */
+ clock_gettime(CLOCK_MONOTONIC, &t1);
+ timespecsub(&t1, &t0, &t2);
+ fprintf(stderr, "%lld.%ld\n", t2.tv_sec, t2.tv_nsec);
}
}
if ((ready & FD_TTY_READY) == 0)masterdiff --git a/pick.c b/pick.c
index 4ba17de..56550fd 100644
--- a/pick.c
+++ b/pick.c
@@ -33,6 +33,8 @@
errx(1, #capability ": unknown terminfo capability"); \
} while (0)
+struct timespec t0, t1, t2;
+
enum key {
UNKNOWN,
ALT_ENTER,
@@ -178,6 +180,7 @@ main(int argc, char *argv[])
err(1, NULL);
}
+ clock_gettime(CLOCK_MONOTONIC, &t0);
input = get_choices();
tty_init(1);
@@ -354,6 +357,9 @@ selected_choice(void)
tty_putp(cursor_normal, 0);
fflush(tty_out);
+ clock_gettime(CLOCK_MONOTONIC, &t1);
+ timespecsub(&t1, &t0, &t2);
+ fprintf(stderr, "%lld.%ld\n", t2.tv_sec, t2.tv_nsec);
switch (get_key(&buf)) {
case ENTER:
if (choices_count > 0) |
$ (echo a; echo aa;) | pick # type: a DOWN a... with latest commit and the diff: |
Codecov Report
@@ Coverage Diff @@
## master #285 +/- ##
=========================================
Coverage ? 89.16%
=========================================
Files ? 1
Lines ? 554
Branches ? 0
=========================================
Hits ? 494
Misses ? 60
Partials ? 0
Continue to review full report at Codecov.
|
This is probably my last remaining annoyance with pick, the fact that it does a blocking read of stdin until EOF prior to entering its main loop. If stdin is slow, this will take time; like running git-log in a huge repository. The new approach uses poll(2) and can therefore read from stdin as new data arrives and simultaneously handle user commands. Some notes about the changes: - get_choices() is made re-entrant. It still allocates a single block of memory for the stdin data. Each choice stores an indirect pointer to this piece of memory since it could be realloacted invalidating any previously stored pointer. This approach is more complex than storing the string in each choice but much faster due to less allocations and strdup(3):ing. - tty_getc() no longer uses the FILE API from libc since it's buffered. Meaning, it could potentially read more input stored in its internal buffer during a single call to getc(3). If all that data is not consumed during the call to get_key(), poll will never flag the underlying file descriptor as non-block readable again since the data is already consumed. Issue exhibited by the test suite. To see it in action: $ while :; do sleep 1; echo bar; done | pick -q bar
If the user already has supplied a query that yields M (where M > 0) matches, inserting the K newly read choices after index M only requires searching through M + K choices instead of N (where N being all choices). This improves performance when the user types a query faster than the input pace on stdin.
|
Reworked the code a bit, but it should not cause any functional difference. Please test! |
|
pick.c: In function ‘main’:
pick.c:332:11: warning: ‘choices_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
size_t choices_offset, cursor_position, i, j, length, nfds,
^so: diff --git a/pick.c b/pick.c
index 9aa9a81..ce42794 100644
--- a/pick.c
+++ b/pick.c
@@ -329,9 +329,9 @@ selected_choice(void)
struct pollfd fds[2];
const char *buf;
ssize_t insert;
- size_t choices_offset, cursor_position, i, j, length, nfds,
- xscroll;
+ size_t cursor_position, i, j, length, nfds, xscroll;
size_t choices_count = 0;
+ size_t choices_offset = 0;
size_t selection = 0;
size_t yscroll = 0;
int dokey, doread, nready, timo; |
|
@mptre Sorry, can't repoduce the issue with #285 (comment) as #285 (comment) shows ... |
|
@DBOTW thanks, try the latest rebased commit. |
|
@mptre No warnings ... but the selection is still reset when new entries stream in that match the query. |
|
@DBOTW one problem at a time 😃, try the latest commit. |
Since polling of character devices is not supported on macOS, add the following work-around: - While stdin has not reached EOF, put the tty in non-blocking mode in order to simulate polling by peeking for new data. - Once stdin has reached EOF, just do a blocking read of the tty.
... assuming it's still in bounds.
|
@mptre Tested and looks good ... 😃
OK, here's the next one: keys that abort diff --git a/pick.c b/pick.c
index db4f407..b2ed42f 100644
--- a/pick.c
+++ b/pick.c
@@ -393,6 +393,7 @@ selected_choice(void)
if (!dokey)
goto render;
+getkey:
switch (get_key(&buf)) {
case ENTER:
if (choices_count > 0)
@@ -561,6 +562,8 @@ render:
&choices_count);
if (dochoices)
dofilter = 0;
+ else
+ goto getkey;
}
tty_putp(cursor_invisible, 0); |
|
I've been running this since the patch was announced, without anything major breaking. Is there any final testing / tweaking we want to do? |
... same here ...
Good question. The requested code reviews are now pending since 3 months ... |
same here I'm experiencing UI-flickering while stdin is being read that I'd like to address before getting this in. |
This is IMHO caused by calling The simplest way is to add a small delay after diff --git a/pick.c b/pick.c
index db4f407..8b797c9 100644
--- a/pick.c
+++ b/pick.c
@@ -589,6 +589,8 @@ render:
tty_putp(tty_parm1(parm_right_cursor, j), 1);
tty_putp(cursor_normal, 0);
fflush(tty_out);
+ if (doread)
+ usleep(10000); /* Try to reduce screen flickering */
}
}
|
I'm thinking instead, we could avoid redrawing the interface when nothing changed. Like when new choices arrive on stdin but they don't affect the choices currently visible. |
Right ... and this is the "core problem": diff --git a/pick.c b/pick.c
index db4f407..af46c40 100644
--- a/pick.c
+++ b/pick.c
@@ -335,7 +335,7 @@ selected_choice(void)
size_t yscroll = 0;
int dokey, doread, nready, timo;
int choices_reset = 1;
- int dochoices = 0;
+ int dochoices = 1;
int dofilter = 1;
cursor_position = query_length;
@@ -484,6 +484,7 @@ selected_choice(void)
break;
case LINE_DOWN:
if (selection < choices_count - 1) {
+ dochoices = 1;
selection++;
if (selection - yscroll == choices_lines)
yscroll++;
@@ -491,6 +492,7 @@ selected_choice(void)
break;
case LINE_UP:
if (selection > 0) {
+ dochoices = 1;
selection--;
if (yscroll > selection)
yscroll--;
@@ -511,19 +513,24 @@ selected_choice(void)
yscroll = selection += choices_lines;
else
selection = choices_count - 1;
+ dochoices = 1;
break;
case PAGE_UP:
if (selection > choices_lines)
yscroll = selection -= choices_lines;
else
yscroll = selection = 0;
+ dochoices = 1;
break;
case END:
- if (choices_count > 0)
+ if (choices_count > 0) {
selection = choices_count - 1;
+ dochoices = 1;
+ }
break;
case HOME:
yscroll = selection = 0;
+ dochoices = 1;
break;
case PRINTABLE:
if (query_length == 0)
@@ -556,7 +563,7 @@ render:
if (choices_reset)
choices_count = choices.length;
choices_reset = 0;
- if (dofilter) {
+ if (dofilter && choices_count > 0) {
dochoices = filter_choices(choices_offset,
&choices_count);
if (dochoices)
@@ -576,6 +585,7 @@ render:
if (selection - yscroll >= choices_lines)
yscroll = selection - choices_lines + 1;
print_choices(yscroll, choices_count, selection);
+ dochoices = 0;
}
tty_putp(carriage_return, 1); /* move cursor to first column */
for (i = j = 0; i < cursor_position; j++) |
|
Tested with: % find $HOME/linux-master -type f | pickNumber of redraws WITHOUT the above patch: ... no UI-flickering or other unnecessary redraws ... /cc @mptre |
|
@DBOTW nice work. I'm still seeing lots of rendering when a query is present and stdin has yet not reached EOF. if we skip rendering when the filitering did not affect the visible choices, I'm seeing a magnitude order of less rendering (experimental diff below). Tried on a large Git-repository: $ (cd /usr/src; git log --oneline) | env LINES=20 ./pick -X -q pledgeThe #define KEY_QUERY 0x1 /* key affects the query */
#define KEY_QUERY_GROW (0x2 | KEY_QUERY) /* key appends to the query */
#define KEY_QUERY_SHRINK (0x4 | KEY_QUERY) /* key makes the query shorter */... then we could infer much of the state from the flags belonging to the pressed key: if ((key->flags & KEY_QUERY))
dofilter = 1;
if ((key->flags & KEY_QUERY_SHRINK))
choices_reset = 1;Anyway, this PR still needs more polishing before being merged. diff --git a/pick.c b/pick.c
index db4f407..1204911 100644
--- a/pick.c
+++ b/pick.c
@@ -111,6 +111,7 @@ static struct {
} choices;
static FILE *tty_in, *tty_out;
static char *query;
+static size_t *choices_order;
static size_t query_length, query_size;
static volatile sig_atomic_t gotsigwinch;
static unsigned int choices_lines, tty_columns, tty_lines;
@@ -204,6 +205,7 @@ main(int argc, char *argv[])
free(*choices.v[0].string);
free(choices.v);
free(query);
+ free(choices_order);
return rc;
}
@@ -335,8 +337,9 @@ selected_choice(void)
size_t yscroll = 0;
int dokey, doread, nready, timo;
int choices_reset = 1;
- int dochoices = 0;
+ int dochoices = 1;
int dofilter = 1;
+ int didkey = 0;
cursor_position = query_length;
@@ -381,10 +384,15 @@ selected_choice(void)
dofilter = 1;
choices_reset = 0;
choices_offset = choices_count;
+ if (didkey)
+ choices_offset = 0;
choices_count +=
choices.length - length;
+ didkey = 0;
} else {
choices_reset = 1;
+ if (choices_count < choices_lines)
+ dofilter = 1;
}
} else {
nfds--; /* EOF */
@@ -484,6 +492,7 @@ selected_choice(void)
break;
case LINE_DOWN:
if (selection < choices_count - 1) {
+ dochoices = 1;
selection++;
if (selection - yscroll == choices_lines)
yscroll++;
@@ -491,6 +500,7 @@ selected_choice(void)
break;
case LINE_UP:
if (selection > 0) {
+ dochoices = 1;
selection--;
if (yscroll > selection)
yscroll--;
@@ -511,19 +521,24 @@ selected_choice(void)
yscroll = selection += choices_lines;
else
selection = choices_count - 1;
+ dochoices = 1;
break;
case PAGE_UP:
if (selection > choices_lines)
yscroll = selection -= choices_lines;
else
yscroll = selection = 0;
+ dochoices = 1;
break;
case END:
- if (choices_count > 0)
+ if (choices_count > 0) {
selection = choices_count - 1;
+ dochoices = 1;
+ }
break;
case HOME:
yscroll = selection = 0;
+ dochoices = 1;
break;
case PRINTABLE:
if (query_length == 0)
@@ -553,6 +568,8 @@ selected_choice(void)
}
render:
+ if (dokey)
+ didkey = 1;
if (choices_reset)
choices_count = choices.length;
choices_reset = 0;
@@ -562,6 +579,8 @@ render:
if (dochoices)
dofilter = 0;
}
+ if (!dochoices && !dokey)
+ continue;
tty_putp(cursor_invisible, 0);
tty_putp(carriage_return, 1); /* move cursor to first column */
@@ -576,6 +595,7 @@ render:
if (selection - yscroll >= choices_lines)
yscroll = selection - choices_lines + 1;
print_choices(yscroll, choices_count, selection);
+ dochoices = 0;
}
tty_putp(carriage_return, 1); /* move cursor to first column */
for (i = j = 0; i < cursor_position; j++)
@@ -607,6 +627,7 @@ filter_choices(size_t offset, size_t *nchoices)
size_t i, match_length;
size_t n = 0;
int nready;
+ int diff = 0;
for (i = offset; i < *nchoices; i++) {
c = &choices.v[i];
@@ -635,7 +656,17 @@ filter_choices(size_t offset, size_t *nchoices)
qsort(choices.v, *nchoices, sizeof(struct choice), choice_cmp);
*nchoices = offset + n;
- return 1;
+ /* Determine if the order of visible choices did change. */
+ /* XXX does not respect yscroll */
+ for (i = 0; i < choices_lines; i++) {
+ if (i == choices.length)
+ break;
+ if (choices_order[i] != choices.v[i].offset)
+ diff = 1;
+ choices_order[i] = choices.v[i].offset;
+ }
+
+ return diff;
}
int
@@ -790,6 +821,12 @@ tty_init(int doinit)
tty_putp(enter_ca_mode, 0);
toggle_sigwinch(0);
+
+ choices_order = reallocarray(choices_order, choices_lines,
+ sizeof(*choices_order));
+ if (choices_order == NULL)
+ err(1, NULL);
+ memset(choices_order, 0, choices_lines * sizeof(*choices_order));
}
int |
|
@mptre slightly modified diff:
diff --git a/pick.c b/pick.c
index db4f407..740f9cb 100644
--- a/pick.c
+++ b/pick.c
@@ -74,6 +74,7 @@ int tty_getc_peek = -1;
static int choice_cmp(const void *, const void *);
static const char *choice_description(const struct choice *);
+static int choice_order(size_t);
static const char *choice_string(const struct choice *);
static void delete_between(char *, size_t, size_t, size_t);
static char *eager_strpbrk(const char *, const char *);
@@ -111,6 +112,7 @@ static struct {
} choices;
static FILE *tty_in, *tty_out;
static char *query;
+static size_t *choices_order;
static size_t query_length, query_size;
static volatile sig_atomic_t gotsigwinch;
static unsigned int choices_lines, tty_columns, tty_lines;
@@ -204,6 +206,7 @@ main(int argc, char *argv[])
free(*choices.v[0].string);
free(choices.v);
free(query);
+ free(choices_order);
return rc;
}
@@ -385,6 +388,8 @@ selected_choice(void)
choices.length - length;
} else {
choices_reset = 1;
+ if (choices_count < choices_lines)
+ dofilter = 1;
}
} else {
nfds--; /* EOF */
@@ -484,6 +489,7 @@ selected_choice(void)
break;
case LINE_DOWN:
if (selection < choices_count - 1) {
+ dochoices = 1;
selection++;
if (selection - yscroll == choices_lines)
yscroll++;
@@ -491,6 +497,7 @@ selected_choice(void)
break;
case LINE_UP:
if (selection > 0) {
+ dochoices = 1;
selection--;
if (yscroll > selection)
yscroll--;
@@ -511,19 +518,24 @@ selected_choice(void)
yscroll = selection += choices_lines;
else
selection = choices_count - 1;
+ dochoices = 1;
break;
case PAGE_UP:
if (selection > choices_lines)
yscroll = selection -= choices_lines;
else
yscroll = selection = 0;
+ dochoices = 1;
break;
case END:
- if (choices_count > 0)
+ if (choices_count > 0) {
selection = choices_count - 1;
+ dochoices = 1;
+ }
break;
case HOME:
yscroll = selection = 0;
+ dochoices = 1;
break;
case PRINTABLE:
if (query_length == 0)
@@ -562,6 +574,8 @@ render:
if (dochoices)
dofilter = 0;
}
+ if (!dochoices && !dokey)
+ continue;
tty_putp(cursor_invisible, 0);
tty_putp(carriage_return, 1); /* move cursor to first column */
@@ -575,7 +589,9 @@ render:
selection = yscroll = 0;
if (selection - yscroll >= choices_lines)
yscroll = selection - choices_lines + 1;
- print_choices(yscroll, choices_count, selection);
+ if (choice_order(yscroll) || dokey)
+ print_choices(yscroll, choices_count, selection);
+ dochoices = 0;
}
tty_putp(carriage_return, 1); /* move cursor to first column */
for (i = j = 0; i < cursor_position; j++)
@@ -670,6 +686,21 @@ choice_description(const struct choice *c)
return *c->string + c->description;
}
+int choice_order(size_t offset) {
+ size_t i;
+ int diff = 0;
+
+ /* Determine if the order of visible choices did change. */
+ for (i = 0; i < choices_lines; i++) {
+ if (i + offset == choices.length)
+ break;
+ if (choices_order[i] != choices.v[i + offset].offset)
+ diff = 1;
+ choices_order[i] = choices.v[i + offset].offset;
+ }
+ return diff;
+}
+
const char *
choice_string(const struct choice *c)
{
@@ -871,6 +902,12 @@ tty_size(void)
tty_lines = 24;
choices_lines = tty_lines - 1; /* available lines, minus query line */
+
+ choices_order = reallocarray(choices_order, choices_lines,
+ sizeof(*choices_order));
+ if (choices_order == NULL)
+ err(1, NULL);
+ memset(choices_order, 0, choices_lines * sizeof(*choices_order));
}
void... |
This is probably my last remaining annoyance with pick, the fact that it does a
blocking read of stdin until EOF prior to entering its main loop. If stdin is
slow, this will take time; like running git-log in a huge repository.
The new approach uses poll(2) and can therefore read from stdin as new data
arrives and simultaneously handle user commands. Some notes about the changes:
get_choices() is made re-entrant. It still allocates a single block of memory
for the stdin data. Each choice stores an indirect pointer to this piece of
memory since it could be realloacted invalidating any previously stored
pointer. This approach is more complex than storing the string in each choice
but much faster due to less allocations and strdup(3):ing.
tty_getc() no longer uses the FILE API from libc since it's buffered. Meaning,
it could potentially read more input stored in its internal buffer during a
single call to getc(3). If all that data is not consumed during the call to
get_key(), poll will never flag the underlying file descriptor as non-block
readable again since the data is already consumed. Issue exhibited by the test
suite.
To see it in action:
Testing and feedback would be much appreciated.
/cc @calleerlandsson @mike-burns @DBOTW @teoljungberg