vt: limit maximum value of params in vt_param_get() #523

Copy link

dnkl commented 2021年05月16日 18:07:43 +02:00

nit/style: for large comments like this, use:

/*
 * This is a very long comment
 */

nit/style: for large comments like this, use: ```c /* * This is a very long comment */ ```

dnkl commented 2021年05月16日 18:29:29 +02:00

I also noted that this:

diff --git a/vt.h b/vt.h
index e4d67c8..033e023 100644
--- a/vt.h
+++ b/vt.h
@@ -17,7 +17,7 @@ vt_param_get(const struct terminal *term, size_t idx, int default_value)
 static_assert(INT_MAX >= 0x7fffffff, "POSIX requires INT_MAX >= 0x7fffffff");
 const unsigned value_mask = 0x7fffffff;
 
- if (term->vt.params.idx > idx) {
+ if (likely(term->vt.params.idx > idx)) {
 unsigned value = term->vt.params.v[idx].value & value_mask;
 return value != 0 ? (int)value : default_value;
 }

generates slightly smaller code:

0000000000000000 <vt_param_get>:
 0: 89 d0 mov %edx,%eax
 2: 0f b6 97 24 05 00 00 movzbl 0x524(%rdi),%edx
 9: 48 39 f2 cmp %rsi,%rdx
 c: 76 14 jbe 22 <vt_param_get+0x22>
 e: 48 8d 14 f6 lea (%rsi,%rsi,8),%rdx
 12: 8b 94 d7 a4 00 00 00 mov 0xa4(%rdi,%rdx,8),%edx
 19: 81 e2 ff ff ff 7f and 0ドルx7fffffff,%edx
 1f: 0f 45 c2 cmovne %edx,%eax
 22: c3 ret

vs.

0000000000000000 <vt_param_get>:
 0: 89 d0 mov %edx,%eax
 2: 0f b6 97 24 05 00 00 movzbl 0x524(%rdi),%edx
 9: 48 39 f2 cmp %rsi,%rdx
 c: 77 02 ja 10 <vt_param_get+0x10>
 e: c3 ret 
 f: 90 nop
 10: 48 8d 14 f6 lea (%rsi,%rsi,8),%rdx
 14: 8b 94 d7 a4 00 00 00 mov 0xa4(%rdi,%rdx,8),%edx
 1b: 81 e2 ff ff ff 7f and 0ドルx7fffffff,%edx
 21: 0f 45 c2 cmovne %edx,%eax
 24: c3 ret

I also noted that this: ```diff diff --git a/vt.h b/vt.h index e4d67c8..033e023 100644 --- a/vt.h +++ b/vt.h @@ -17,7 +17,7 @@ vt_param_get(const struct terminal *term, size_t idx, int default_value) static_assert(INT_MAX >= 0x7fffffff, "POSIX requires INT_MAX >= 0x7fffffff"); const unsigned value_mask = 0x7fffffff; - if (term->vt.params.idx > idx) { + if (likely(term->vt.params.idx > idx)) { unsigned value = term->vt.params.v[idx].value & value_mask; return value != 0 ? (int)value : default_value; } ``` generates slightly smaller code: ``` 0000000000000000 <vt_param_get>: 0: 89 d0 mov %edx,%eax 2: 0f b6 97 24 05 00 00 movzbl 0x524(%rdi),%edx 9: 48 39 f2 cmp %rsi,%rdx c: 76 14 jbe 22 <vt_param_get+0x22> e: 48 8d 14 f6 lea (%rsi,%rsi,8),%rdx 12: 8b 94 d7 a4 00 00 00 mov 0xa4(%rdi,%rdx,8),%edx 19: 81 e2 ff ff ff 7f and 0ドルx7fffffff,%edx 1f: 0f 45 c2 cmovne %edx,%eax 22: c3 ret ``` vs. ``` 0000000000000000 <vt_param_get>: 0: 89 d0 mov %edx,%eax 2: 0f b6 97 24 05 00 00 movzbl 0x524(%rdi),%edx 9: 48 39 f2 cmp %rsi,%rdx c: 77 02 ja 10 <vt_param_get+0x10> e: c3 ret f: 90 nop 10: 48 8d 14 f6 lea (%rsi,%rsi,8),%rdx 14: 8b 94 d7 a4 00 00 00 mov 0xa4(%rdi,%rdx,8),%edx 1b: 81 e2 ff ff ff 7f and 0ドルx7fffffff,%edx 21: 0f 45 c2 cmovne %edx,%eax 24: c3 ret ```

craigbarnes commented 2021年05月16日 21:14:12 +02:00

I'm not so sure if this branch really is "likely", even though the code does look a bit cleaner. 033円[K and 033円[m seem quite common in TUI programs. I think it's a slippery slope to use it based on specific results, rather than as part of a covenant with the compiler.

I'm not so sure if this branch really is "likely", even though the code does look a bit cleaner. `033円[K` and `033円[m` seem quite common in TUI programs. I think it's a slippery slope to use it based on specific results, rather than as part of a covenant with the compiler.

craigbarnes commented 2021年05月16日 21:23:51 +02:00

Hmm, on second thoughts, most shells and curses programs do seem to generate a lot more CSI sequences with params than without them. I've added this as an additional commit.

Hmm, on second thoughts, most shells and curses programs do seem to generate a *lot* more CSI sequences with params than without them. I've added this as an additional commit.

dnkl commented 2021年05月16日 21:24:47 +02:00

Ah, ok :)

Ah, ok :)

dnkl commented 2021年05月16日 21:23:49 +02:00

Did we misunderstand each other? When I benched this I didn't see any improvements at all, plus, I agree with you, sequences that rely on the default value is probably very common.

Did we misunderstand each other? When I benched this I didn't see any improvements at all, plus, I agree with you, sequences that rely on the default value is probably **very** common.

craigbarnes commented 2021年05月16日 21:25:02 +02:00

Oops. No, I was mid way through adding it when I saw your reply. I'll revert it.

Oops. No, I was mid way through adding it when I saw your reply. I'll revert it.

dnkl commented 2021年05月16日 21:25:08 +02:00

Just saw your other comment, ignore this :)

Just saw your other comment, ignore this :)