Skip to content

Extract structure of key interpretation from the side effect of reading keystrokes#29

Merged
noelwelsh merged 6 commits intocreativescala:mainfrom
kapunga:key-reader-adt
May 27, 2025
Merged

Extract structure of key interpretation from the side effect of reading keystrokes#29
noelwelsh merged 6 commits intocreativescala:mainfrom
kapunga:key-reader-adt

Conversation

@kapunga
Copy link
Contributor

@kapunga kapunga commented Apr 18, 2025

Refactors readKey() to interpret a structure instead of having the key mappings embedded in the function. This makes the structure re-usable for alternate interpreters.

I'm open to re-naming and re-ordering anything. Full disclosure, re-structuring everything looked like a bit of a slog, so I used Claude Code to extract all of the escape sequences.

Resolves: #28


import terminus.effect.Ascii

class KeySequence(val root: Key, val sequences: Map[String, Key]):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to make this less generic. My thinking here is maybe down the line, it might be nice to support telnet control sequences (They start with '\x00ff'), or maybe someone might want to build custom control sequences into an application.

case '=' => Key('≠')
case other: Char => Key.unknown(s"${Ascii.ESC}${other}")
@tailrec
private def readKeySequence(acc: String, sequence: KeySequence): Eof | Key = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem this still has that the previous implementation has is that if it picks up a second user keystroke in < 100ms, it could get swallowed up.

Copy link
Contributor

@noelwelsh noelwelsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the impact of this change on performance. Reading keys is likely to be in the inner loop of many applications, and I think this change adds of lot of overhead.

I think the intention here is to use something very close to a trie, aka prefix tree, and I think the implementation would be clearer if that was made explicit.

kapunga added 5 commits May 2, 2025 22:46
Create an inital structure for key code interpretation.
Split keycode structure into base keys and escapeSequences.
Create escapeSubsequnces to support key parsing.
@kapunga
Copy link
Contributor Author

kapunga commented May 4, 2025

I'm a bit concerned about the impact of this change on performance. Reading keys is likely to be in the inner loop of many applications, and I think this change adds of lot of overhead.

I think the intention here is to use something very close to a trie, aka prefix tree, and I think the implementation would be clearer if that was made explicit.

I chose this structure over a trie for a couple of reasons.

  1. I wanted to make it easy to see existing codes in their entirety when examining the file. If you are trying to visually scan the file to see if a particular escape sequence is include, for example s"$ESC[6;5~" -> Key.controlPageDown, it is much easier to see.
  2. I wanted the root of the escape sequences to be different from the intermediate components. As I worked on a draft implementation of making a fs2.Stream[F, Key], I found that at the root, I needed the entry at ESC to both contain Key.Escape and all of the escape sequences, so that if the input was something like ESC -> ESC -> [ -> A in < 100ms, I could release Key.Escape once the second ESC arrives.

As for performance:

  • My intuition was that my structure would be more space performant than an explicit TRIE implementation as it wouldn't need a lot of intermediate Mapinstances. This implementation only has two Map instances and a Set. I think this is important as a lot of the use cases are likely to involve command line tools, and I believe that it's important to keep the memory footprint of those down as much as possible. I have not confirmed the running memory footprint difference yet.
  • My intuition was that this structure would be more speed performant than the existing implementation as it eliminates the usage of match statements which I believe would underperform at most two Map/Set lookups per input character. To test this assumption, I spun up a benchmarking branch ([WIP] Key reader benchmark #33) and the results show ~40-50x speed increase over the existing implementation:
// Existing implementation
[info] Benchmark                                        Mode  Cnt  Score   Error  Units
[info] TerminalKeyReaderBenchmark.benchmarkLargeInput   avgt   20  9.058 ± 0.069  ms/op
[info] TerminalKeyReaderBenchmark.benchmarkMediumInput  avgt   20  4.511 ± 0.027  ms/op
[info] TerminalKeyReaderBenchmark.benchmarkSmallInput   avgt   20  0.902 ± 0.003  ms/op

// Implementation in this PR
[info] Benchmark                                        Mode  Cnt  Score    Error  Units
[info] TerminalKeyReaderBenchmark.benchmarkLargeInput   avgt   20  0.217 ±  0.014  ms/op
[info] TerminalKeyReaderBenchmark.benchmarkMediumInput  avgt   20  0.089 ±  0.007  ms/op
[info] TerminalKeyReaderBenchmark.benchmarkSmallInput   avgt   20  0.016 ±  0.001  ms/op

@kapunga kapunga requested a review from noelwelsh May 4, 2025 14:55
@noelwelsh
Copy link
Contributor

I don't have time to look at this at the moment, but just wanted to let you know I appreciate the work. I should have some time for an in-depth look later next week.

@noelwelsh
Copy link
Contributor

Ok, I can't really argument with the performance improvement! Also interesting that it didn't match my mental performance at all.

This seems like a win all around then.

@noelwelsh noelwelsh merged commit 2224e66 into creativescala:main May 27, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor KeyReader to allow for different interpreters

2 participants