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

Added APC support #110

Closed
wants to merge 10 commits into from
4 changes: 4 additions & 0 deletions examples/parselog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ impl Perform for Log {
intermediates, ignore, byte
);
}

fn apc_dispatch(&mut self, bytes: &[u8]) {
println!("[apc_dispatch] {:?}", bytes);
}
}

fn main() {
Expand Down
77 changes: 7 additions & 70 deletions src/definitions.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use core::mem;

#[allow(dead_code)]
#[repr(u8)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Debug, Default, Copy, Clone, PartialEq, Eq)]
pub enum State {
Anywhere = 0,
CsiEntry = 1,
Expand All @@ -19,13 +17,14 @@ pub enum State {
#[default]
Ground = 12,
OscString = 13,
SosPmApcString = 14,
SosPmString = 14,
Utf8 = 15,
ApcString = 16,
}

#[allow(dead_code)]
#[repr(u8)]
#[derive(Debug, Clone, Copy)]
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum Action {
None = 0,
Clear = 1,
Expand All @@ -43,69 +42,7 @@ pub enum Action {
Put = 13,
Unhook = 14,
BeginUtf8 = 15,
}

/// Unpack a u8 into a State and Action
///
/// The implementation of this assumes that there are *precisely* 16 variants for both Action and
/// State. Furthermore, it assumes that the enums are tag-only; that is, there is no data in any
/// variant.
///
/// Bad things will happen if those invariants are violated.
#[inline(always)]
pub fn unpack(delta: u8) -> (State, Action) {
unsafe {
(
// State is stored in bottom 4 bits
mem::transmute(delta & 0x0f),
// Action is stored in top 4 bits
mem::transmute(delta >> 4),
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be benchmarked. There is no way we'll merge a patch that reduces performance for a feature nobody should be using.

Copy link
Author

Choose a reason for hiding this comment

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

Running some initial benchmarks I've got the following results from cargo bench:

test bench::state_changes ... bench:     108,197 ns/iter (+/- 3,199)
test bench::testfile      ... bench:     121,021 ns/iter (+/- 3,617)

whilst the old code yields the following:

test bench::state_changes ... bench:      95,320 ns/iter (+/- 4,951)
test bench::testfile      ... bench:     101,009 ns/iter (+/- 7,342)

Which equates to roughly an 12% ~ 16% decrease in performance. I am using flamegraph now to investigate.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also recommend testing with https://github.com/alacritty/vtebench. That's ultimately what decides which features get merged into Alacritty or not.


#[inline(always)]
pub const fn pack(state: State, action: Action) -> u8 {
(action as u8) << 4 | state as u8
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn unpack_state_action() {
match unpack(0xee) {
(State::SosPmApcString, Action::Unhook) => (),
_ => panic!("unpack failed"),
}

match unpack(0x0f) {
(State::Utf8, Action::None) => (),
_ => panic!("unpack failed"),
}

match unpack(0xff) {
(State::Utf8, Action::BeginUtf8) => (),
_ => panic!("unpack failed"),
}
}

#[test]
fn pack_state_action() {
match unpack(0xee) {
(State::SosPmApcString, Action::Unhook) => (),
_ => panic!("unpack failed"),
}

match unpack(0x0f) {
(State::Utf8, Action::None) => (),
_ => panic!("unpack failed"),
}

match unpack(0xff) {
(State::Utf8, Action::BeginUtf8) => (),
_ => panic!("unpack failed"),
}
}
ApcStart = 16,
ApcEnd = 17,
ApcPut = 18,
}
73 changes: 69 additions & 4 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ mod table;
pub mod ansi;
pub use params::{Params, ParamsIter};

use definitions::{unpack, Action, State};
use definitions::{Action, State};

const MAX_INTERMEDIATES: usize = 2;
const MAX_OSC_PARAMS: usize = 16;
Expand All @@ -73,7 +73,8 @@ impl<'a, P: Perform> utf8::Receiver for VtUtf8Receiver<'a, P> {
/// [`Perform`]: trait.Perform.html
///
/// Generic over the value for the size of the raw Operating System Command
/// buffer. Only used when the `no_std` feature is enabled.
/// buffer. Only used when the `no_std` feature is enabled. This buffer is
/// also used for APC (Application Program Commands.)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// also used for APC (Application Program Commands.)
/// also used for APC (Application Program Commands).

#[derive(Default)]
pub struct Parser<const OSC_RAW_BUF_SIZE: usize = MAX_OSC_RAW> {
state: State,
Expand Down Expand Up @@ -138,12 +139,12 @@ impl<const OSC_RAW_BUF_SIZE: usize> Parser<OSC_RAW_BUF_SIZE> {
// for current state.
let mut change = table::STATE_CHANGES[State::Anywhere as usize][byte as usize];

if change == 0 {
if change == (State::Anywhere, Action::None) {
change = table::STATE_CHANGES[self.state as usize][byte as usize];
}

// Unpack into a state and action
let (state, action) = unpack(change);
let (state, action) = change;

self.perform_state_change(performer, state, action, byte);
}
Expand Down Expand Up @@ -187,6 +188,9 @@ impl<const OSC_RAW_BUF_SIZE: usize> Parser<OSC_RAW_BUF_SIZE> {
State::OscString => {
self.perform_action(performer, Action::OscEnd, byte);
},
State::ApcString => {
self.perform_action(performer, Action::ApcEnd, byte);
},
_ => (),
}

Expand All @@ -202,6 +206,9 @@ impl<const OSC_RAW_BUF_SIZE: usize> Parser<OSC_RAW_BUF_SIZE> {
State::OscString => {
self.perform_action(performer, Action::OscStart, byte);
},
State::ApcString => {
self.perform_action(performer, Action::ApcStart, byte);
},
_ => (),
}

Expand Down Expand Up @@ -231,6 +238,10 @@ impl<const OSC_RAW_BUF_SIZE: usize> Parser<OSC_RAW_BUF_SIZE> {
}
}

fn apc_dispatch<P: Perform>(&self, performer: &mut P) {
performer.apc_dispatch(&self.osc_raw);
}

#[inline]
fn perform_action<P: Perform>(&mut self, performer: &mut P, action: Action, byte: u8) {
match action {
Expand Down Expand Up @@ -364,6 +375,22 @@ impl<const OSC_RAW_BUF_SIZE: usize> Parser<OSC_RAW_BUF_SIZE> {
Action::BeginUtf8 => self.process_utf8(performer, byte),
Action::Ignore => (),
Action::None => (),
Action::ApcStart => {
self.osc_raw.clear();
},
Action::ApcEnd => {
self.apc_dispatch(performer);
},
Copy link
Member

Choose a reason for hiding this comment

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

Statements should be inlined.

Action::ApcPut => {
#[cfg(feature = "no_std")]
{
if self.osc_raw.is_full() {
return;
}
}

self.osc_raw.push(byte);
},
}
}
}
Expand Down Expand Up @@ -428,6 +455,10 @@ pub trait Perform {
/// The `ignore` flag indicates that more than two intermediates arrived and
/// subsequent characters were ignored.
fn esc_dispatch(&mut self, _intermediates: &[u8], _ignore: bool, _byte: u8) {}

/// Called at the end of an APC (application program command), where
/// `bytes` is the content of the APC
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Called at the end of an APC (application program command), where
/// `bytes` is the content of the APC
/// Called at the end of an APC (application program command), where
/// `bytes` is the content of the APC.

fn apc_dispatch(&mut self, _bytes: &[u8]) {}
}

#[cfg(all(test, feature = "no_std"))]
Expand Down Expand Up @@ -459,6 +490,7 @@ mod tests {
Esc(Vec<u8>, bool, u8),
DcsHook(Vec<Vec<u16>>, Vec<u8>, bool, char),
DcsPut(u8),
Apc(Vec<u8>),
DcsUnhook,
}

Expand Down Expand Up @@ -492,6 +524,10 @@ mod tests {
fn unhook(&mut self) {
self.dispatched.push(Sequence::DcsUnhook);
}

fn apc_dispatch(&mut self, bytes: &[u8]) {
self.dispatched.push(Sequence::Apc(bytes.to_vec()))
}
}

#[test]
Expand Down Expand Up @@ -880,6 +916,35 @@ mod tests {
assert_eq!(dispatcher.dispatched[6], Sequence::DcsUnhook);
}

#[test]
fn parse_apc() {
const INPUT_1: &[u8] = b"\x1b_abc\x9c";
const INPUT_2: &[u8] = b"\x1b_abc\x1b\\";

// Test with ST terminator
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Test with ST terminator
// Test with ST terminator.


let mut dispatcher = Dispatcher::default();
let mut parser = Parser::new();

for byte in INPUT_1 {
parser.advance(&mut dispatcher, *byte);
}
assert_eq!(dispatcher.dispatched, vec![Sequence::Apc(b"abc".to_vec()),]);

// Test with ESC \ terminator
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Test with ESC \ terminator
// Test with ESC \ terminator.


let mut dispatcher = Dispatcher::default();
let mut parser = Parser::new();

for byte in INPUT_2 {
parser.advance(&mut dispatcher, *byte);
}
assert_eq!(dispatcher.dispatched, vec![
Sequence::Apc(b"abc".to_vec()),
Sequence::Esc(vec![], false, 92)
])
}

#[test]
fn intermediate_reset_on_dcs_exit() {
static INPUT: &[u8] = b"\x1bP=1sZZZ\x1b+\x5c";
Expand Down
29 changes: 19 additions & 10 deletions src/table.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/// This is the state change table. It's indexed first by current state and then by the next
/// character in the pty stream.
use crate::definitions::{pack, Action, State};
use crate::definitions::{Action, State};

use vte_generate_state_changes::generate_state_changes;

// Generate state changes at compile-time
pub static STATE_CHANGES: [[u8; 256]; 16] = state_changes();
pub static STATE_CHANGES: [[(State, Action); 256]; 17] = state_changes();
generate_state_changes!(state_changes, {
Anywhere {
0x18 => (Ground, Execute),
Expand Down Expand Up @@ -44,9 +44,9 @@ generate_state_changes!(state_changes, {
0x5b => (CsiEntry, None),
0x5d => (OscString, None),
0x50 => (DcsEntry, None),
0x58 => (SosPmApcString, None),
0x5e => (SosPmApcString, None),
0x5f => (SosPmApcString, None),
0x58 => (SosPmString, None),
0x5e => (SosPmString, None),
0x5f => (ApcString, None),
},

EscapeIntermediate {
Expand Down Expand Up @@ -152,11 +152,11 @@ generate_state_changes!(state_changes, {
0x9c => (Ground, None),
},

SosPmApcString {
0x00..=0x17 => (Anywhere, Ignore),
0x19 => (Anywhere, Ignore),
0x1c..=0x1f => (Anywhere, Ignore),
0x20..=0x7f => (Anywhere, Ignore),
SosPmString {
0x00..=0x17 => (SosPmString, Ignore),
0x19 => (SosPmString, Ignore),
0x1c..=0x1f => (SosPmString, Ignore),
0x20..=0x7f => (SosPmString, Ignore),
0x9c => (Ground, None),
},

Expand All @@ -168,4 +168,13 @@ generate_state_changes!(state_changes, {
0x1c..=0x1f => (Anywhere, Ignore),
0x20..=0xff => (Anywhere, OscPut),
}

ApcString {
0x00..=0x06 => (Anywhere, Ignore),
0x08..=0x17 => (Anywhere, Ignore),
0x19 => (Anywhere, Ignore),
0x1c..=0x1f => (Anywhere, Ignore),
0x20..=0xff => (Anywhere, ApcPut),
0x9c => (Ground, None),
Copy link
Member

Choose a reason for hiding this comment

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

Haven't checked these yet, just for personal reference.

},
});
7 changes: 3 additions & 4 deletions vte_generate_state_changes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ pub fn generate_state_changes(item: proc_macro::TokenStream) -> proc_macro::Toke
let assignments_stream = states_stream(&mut iter);

quote!(
const fn #fn_name() -> [[u8; 256]; 16] {
let mut state_changes = [[0; 256]; 16];
const fn #fn_name() -> [[(State, Action); 256]; 17] {
let mut state_changes = [[(State::Anywhere, Action::None); 256]; 17];

#assignments_stream

Expand Down Expand Up @@ -102,10 +102,9 @@ fn change_stream(iter: &mut Peekable<token_stream::IntoIter>, state: &TokenTree)
// Create a new entry for every byte in the range
for byte in start..=end {
// TODO: Force adding `State::` and `Action::`?
// TODO: Should we really use `pack` here without import?
tokens.extend(quote!(
state_changes[State::#state as usize][#byte] =
pack(State::#target_state, Action::#target_action);
(State::#target_state, Action::#target_action);
));
}
}
Expand Down