psychon
commented
4 years ago Random ramblings
> Event::parse is more than 9kb of code (this is only with randr) I took a look at the output of `objdump -S target/release/examples/xclock_utc` (this is without any extensions enabled). Looks like a lot was inlined into this function. For master, I get 1431 lines of assembly code. This is surely less than 9 KiB of binary code, but I do not have randr enabled. With the patch from my last comment... uhm... nothing changed (well, the number of lines did not change). Weird. With `cargo build --release --example xclock_utc --features all-extensions`, this function turns into a 6537-lines-monster. Hm... When scrolling through this, a lot of the code looks like bad versions of `memcpy`, i.e. stuff like this: ``` 2f87e: 8b 8c 24 97 00 00 00 mov 0x97(%rsp),%ecx 2f885: 89 8c 24 67 02 00 00 mov %ecx,0x267(%rsp) 2f88c: 48 8b 8c 24 90 00 00 mov 0x90(%rsp),%rcx 2f893: 00 2f894: 48 89 8c 24 60 02 00 mov %rcx,0x260(%rsp) 2f89b: 00 2f89c: 8b 8c 24 67 02 00 00 mov 0x267(%rsp),%ecx 2f8a3: 89 8c 24 87 01 00 00 mov %ecx,0x187(%rsp) 2f8aa: 48 8b 8c 24 60 02 00 mov 0x260(%rsp),%rcx 2f8b1: 00 2f8b2: 48 89 8c 24 80 01 00 mov %rcx,0x180(%rsp) 2f8b9: 00 2f8ba: 48 8b 8c 24 80 01 00 mov 0x180(%rsp),%rcx 2f8c1: 00 2f8c2: 48 89 8c 24 d3 01 00 mov %rcx,0x1d3(%rsp) 2f8c9: 00 2f8ca: 8b 8c 24 87 01 00 00 mov 0x187(%rsp),%ecx 2f8d1: 89 8c 24 da 01 00 00 mov %ecx,0x1da(%rsp) 2f8d8: 48 8b 8c 24 d0 01 00 mov 0x1d0(%rsp),%rcx 2f8df: 00 2f8e0: 49 89 4e 01 mov %rcx,0x1(%r14) 2f8e4: 48 8b 8c 24 d6 01 00 mov 0x1d6(%rsp),%rcx 2f8eb: 00 2f8ec: 49 89 4e 07 mov %rcx,0x7(%r14) 2f8f0: 41 88 46 0f mov %al,0xf(%r14) ``` Could this come from lines like ` xproto::BUTTON_PRESS_EVENT => return Ok(Self::ButtonPress(event.try_into()?)),` where the compiler copies the result from `event.try_into()?` into the newly constructed enum? Adding the following to `Cargo.toml` ``` [profile.release] opt-level = 'z' ``` turns that 6537-lines-monster into a 3442-lines-monster. And there are now 44 calls to `memcpy` (the old code had 26). No idea where 44 comes from. And the assembly still contains unrolled `memcpy`s... :-/Could it be that this big code size comes from "moving large things into an enum with many variants"?
Edit: Seems like this kind of pattern does indeed generate way too much code: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=ee3bd9d4445b7657ee94dfec319a88ee
Edit: Yup. https://stackoverflow.com/questions/39219961/how-to-get-assembly-output-from-building-with-cargo taught me to use cargo rustc --release -- --emit asm. That is a lot more readable than the objdump output. Plus, I can actually see the jump table that is used. So this code does indeed have this structure:
preparation
look up entry in jump table
jump
case 1: well, this calls `Error::parse` and is special.
case 2:
shuffle registers around
call `KeyPressEvent::try_from`
lots of code to copy around memory
check if it failed
more memory is copied around
done
[This is 28 lines of assembly for doing almost nothing]
case 3:
Basically the same as case 2, but duplicated. The same for all the following cases
[More code follows, e.g. error handling, handling generic events, handling the "Unknown" case"]Edit: Hm... This code does not look like it went through compiler optimisations. Nothing is inlined in KeyPressEvent::try_parse. Does --release not work for cargo rustc?
Edit: cargo rustc --release -- -Copt-level=s --emit asm does better.
Edit: as -o /tmp/t.o the-file-generated-by-the-previous-command.s can generate an object file. That way, one can look at the binary size (and run size on some file). Running strip /tmp/t.o still does some progress: The file size goes from 2.4 MiB to 1.3 MiB.
jneem
https://github.com/xi-editor/druid/pull/1025#issuecomment-644448962
I'm not sure I understand this correctly (is it "there are multiple things (e.g.
KeyPressEvent::try_parse) that are in total 3kb in size" or is it "KeyPressEvent::try_parseis 3kb in size"?), but I cannot really reproduce.I copied together some self-contained code for `KeyPressEvent::try_parse`
```rust use std::convert::TryInto; pub enum ParseError { ParseError } pub type Window = u32; pub type Pixmap = u32; pub type Cursor = u32; pub type Font = u32; pub type Gcontext = u32; pub type Colormap = u32; pub type Atom = u32; pub type Drawable = u32; pub type Fontable = u32; pub type Bool32 = u32; pub type Visualid = u32; pub type Timestamp = u32; pub type Keysym = u32; pub type Keycode = u8; pub type Keycode32 = u32; pub type Button = u8; /// A type implementing this trait can be parsed from some raw bytes. pub trait TryParse: Sized { /// Try to parse the given values into an instance of this type. /// /// If parsing is successful, an instance of the type and a slice for the remaining data should /// be returned. Otherwise, an error is returned. fn try_parse(value: &[u8]) -> Result<(Self, &[u8]), ParseError>; } macro_rules! implement_try_parse { ($t:ty) => { impl TryParse for $t { fn try_parse(value: &[u8]) -> Result<(Self, &[u8]), ParseError> { let len = std::mem::size_of::<$t>(); let bytes = value .get(..len) .ok_or(ParseError::ParseError)? .try_into() // TryInto<[u8; len]> .unwrap(); Ok((<$t>::from_ne_bytes(bytes), &value[len..])) } } }; } impl TryParse for bool { fn try_parse(value: &[u8]) -> Result<(Self, &[u8]), ParseError> { let (data, remaining) = u8::try_parse(value)?; Ok((data != 0, remaining)) } } implement_try_parse!(u8); implement_try_parse!(i8); implement_try_parse!(u16); implement_try_parse!(i16); implement_try_parse!(u32); implement_try_parse!(i32); implement_try_parse!(u64); implement_try_parse!(i64); pub struct KeyPressEvent { pub response_type: u8, pub detail: Keycode, pub sequence: u16, pub time: Timestamp, pub root: Window, pub event: Window, pub child: Window, pub root_x: i16, pub root_y: i16, pub event_x: i16, pub event_y: i16, pub state: u16, pub same_screen: bool, } impl TryParse for KeyPressEvent { fn try_parse(initial_value: &[u8]) -> Result<(Self, &[u8]), ParseError> { let remaining = initial_value; let (response_type, remaining) = u8::try_parse(remaining)?; let (detail, remaining) = Keycode::try_parse(remaining)?; let (sequence, remaining) = u16::try_parse(remaining)?; let (time, remaining) = Timestamp::try_parse(remaining)?; let (root, remaining) = Window::try_parse(remaining)?; let (event, remaining) = Window::try_parse(remaining)?; let (child, remaining) = Window::try_parse(remaining)?; let (root_x, remaining) = i16::try_parse(remaining)?; let (root_y, remaining) = i16::try_parse(remaining)?; let (event_x, remaining) = i16::try_parse(remaining)?; let (event_y, remaining) = i16::try_parse(remaining)?; let (state, remaining) = u16::try_parse(remaining)?; let (same_screen, remaining) = bool::try_parse(remaining)?; let remaining = remaining.get(1..).ok_or(ParseError::ParseError)?; let result = KeyPressEvent { response_type, detail, sequence, time, root, event, child, root_x, root_y, event_x, event_y, state, same_screen }; let _ = remaining; let remaining = initial_value.get(32..) .ok_or(ParseError::ParseError)?; Ok((result, remaining)) } } ```The resulting compiler output with
-Copt-level=3 --edition=2018is 41 KiB of text (according to https://rust.godbolt.org).I cannot easily see the binary size, but here is the assembly for `KeyPressEvent::try_parse`:
```rustThat's just 90 lines of assembly and it does not call any other code. This can't be 3 KiB of binary code.
Without optimisation, the output is a lot more ugly, but I do not think that looking at this output makes sense.
One thing I notice: llvm managed to merge all the error handling, but it does not notice that it can simplify
if length < 4 then goto error; if length < 8 then goto error;etc. Addingif initial_value.len() < 32 { return Err(ParseError::ParseError); }as a new first line toKeyPressEvent::try_parsehelps here. The assembly now only has 56 lines. There are some simplifications that I do not immediately understand, but all of this "cmpwith small number, then jump" was merged into a singlecmp rdx, 31. I guess generating something like this "everywhere" in the code generator shouldn't be too hard and should help a lot.For the timeline: Optimisation just for the sake of optimisation is hard. It makes more sense to take "size of some program" as a measurement. Thus, I suggest not to merge anything on this before the release and instead proceed carefully.
A goal for optimisation might be to take one of the examples in this repo and check their binary size. For example
cargo build --release --example xclock_utcresults in a 7.3 MiB binary whichstripturns into 503 KiB. After the following patch, this turns into 7.3 MiB and 499 KiB. That's already 4 KiB less, just by adding more code to the generated code. :-)CC @jneem I'd be happy about your input here. (And I have never worked with
cargo bloatbefore.) One quick question: Did you usecargo build --release? Or did I perhaps misunderstand you?