From 39be1861642ccb9856e1e1191f5bf5769cd4f169 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 3 Feb 2024 13:42:23 +0100 Subject: [PATCH 1/2] Optimise atom_manager! implementation winit uses atom_manager! with 59 atom names. Due to some sub-optimal code generation, this causes the generated new() function to end up with a size of 111.6 KiB, or almost 5% of the .text size of a winit example. The issue is that each "intern_atom()?" can cause an early exit and needs to call drop() of the already generated Cookies. This leads to some quadratic growth. To work around this, in this commit I change the implementation of new(). It now iterates over the atom names, using .map() to send the InternAtom requests. Finally, collect() is then used to change this Iterator> into Result, Error>. This even preserves the early-exit behaviour in case of errors. The reply() function is then changed to turn the Vec into an iterator, using next() to pull out the items. This relies on the evaluation of this code to be well-defined (which e.g. wouldn't be guaranteed in C). Luckily, Rust provides the needed guarantee: https://github.com/rust-lang/reference/pull/888 Fixes: https://github.com/psychon/x11rb/issues/912 Signed-off-by: Uli Schlachter --- x11rb/src/x11_utils.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/x11rb/src/x11_utils.rs b/x11rb/src/x11_utils.rs index 55b4980f..e339c84a 100644 --- a/x11rb/src/x11_utils.rs +++ b/x11rb/src/x11_utils.rs @@ -44,17 +44,19 @@ pub use x11rb_protocol::x11_utils::{ /// #[derive(Debug)] /// /// A handle to a response from the X11 server. /// struct AtomCollectionCookie<'c, C: ConnectionExt> { -/// phantom: std::marker::PhantomData<&'c C>, -/// _NET_WM_NAME: Cookie<'c, C, InternAtomReply>, -/// _NET_WM_ICON: Cookie<'c, C, InternAtomReply>, -/// ATOM_WITH_SPACES: Cookie<'c, C, InternAtomReply>, -/// WHATEVER: Cookie<'c, C, InternAtomReply>, +/// // please treat the actual members as private +/// # phantom: std::marker::PhantomData<&'c C>, +/// # _NET_WM_NAME: Cookie<'c, C, InternAtomReply>, +/// # _NET_WM_ICON: Cookie<'c, C, InternAtomReply>, +/// # ATOM_WITH_SPACES: Cookie<'c, C, InternAtomReply>, +/// # WHATEVER: Cookie<'c, C, InternAtomReply>, /// } /// /// impl AtomCollection { /// pub fn new( /// conn: &C, /// ) -> Result, ConnectionError> { +/// // This is just an example for readability; the actual code is more efficient. /// Ok(AtomCollectionCookie { /// phantom: std::marker::PhantomData, /// _NET_WM_NAME: conn.intern_atom(false, b"_NET_WM_NAME")?, @@ -70,6 +72,7 @@ pub use x11rb_protocol::x11_utils::{ /// C: ConnectionExt, /// { /// pub fn reply(self) -> Result { +/// // This is just an example for readability; the actual code is different. /// Ok(AtomCollection { /// _NET_WM_NAME: self._NET_WM_NAME.reply()?.atom, /// _NET_WM_ICON: self._NET_WM_ICON.reply()?.atom, @@ -94,10 +97,8 @@ macro_rules! atom_manager { #[derive(Debug)] $(#[$cookie_meta])* $vis struct $cookie_name<'a, C: $crate::protocol::xproto::ConnectionExt> { - phantom: std::marker::PhantomData<&'a C>, - $( - $field_name: $crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>, - )* + __private_phantom: std::marker::PhantomData<&'a C>, + __private_cookies: Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>, } // Replies @@ -114,23 +115,23 @@ macro_rules! atom_manager { $vis fn new( _conn: &C, ) -> ::std::result::Result<$cookie_name<'_, C>, $crate::errors::ConnectionError> { + let names = [ + $($crate::__atom_manager_atom_value!($field_name$(: $atom_value)?),)* + ]; + let cookies: Result, _> = names.into_iter().map(|name| _conn.intern_atom(false, name)).collect(); Ok($cookie_name { - phantom: std::marker::PhantomData, - $( - $field_name: _conn.intern_atom( - false, - $crate::__atom_manager_atom_value!($field_name$(: $atom_value)?), - )?, - )* + __private_phantom: std::marker::PhantomData, + __private_cookies: cookies?, }) } } impl<'a, C: $crate::protocol::xproto::ConnectionExt> $cookie_name<'a, C> { $vis fn reply(self) -> ::std::result::Result<$struct_name, $crate::errors::ReplyError> { + let mut replies = self.__private_cookies.into_iter(); Ok($struct_name { $( - $field_name: self.$field_name.reply()?.atom, + $field_name: replies.next().expect("new() should have constructed a Vec of the correct size").reply()?.atom, )* }) } From f431b67d81389d8a59f70f66722224acf40ec1a2 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Mon, 5 Feb 2024 16:27:12 +0100 Subject: [PATCH 2/2] Test & fix atom_manager! macro hygiene This makes sure that the macro refers to standard types in an unambiguous way that does not use types from the local module. Instead, a full path beginning with "::std" is used. Thanks to @notgull for pointing this out. Signed-off-by: Uli Schlachter --- x11rb/src/x11_utils.rs | 9 ++++--- x11rb/tests/x11_utils.rs | 54 ++++++++++++++++++++++++++-------------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/x11rb/src/x11_utils.rs b/x11rb/src/x11_utils.rs index e339c84a..45de36aa 100644 --- a/x11rb/src/x11_utils.rs +++ b/x11rb/src/x11_utils.rs @@ -97,8 +97,8 @@ macro_rules! atom_manager { #[derive(Debug)] $(#[$cookie_meta])* $vis struct $cookie_name<'a, C: $crate::protocol::xproto::ConnectionExt> { - __private_phantom: std::marker::PhantomData<&'a C>, - __private_cookies: Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>, + __private_phantom: ::std::marker::PhantomData<&'a C>, + __private_cookies: ::std::vec::Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>, } // Replies @@ -118,9 +118,10 @@ macro_rules! atom_manager { let names = [ $($crate::__atom_manager_atom_value!($field_name$(: $atom_value)?),)* ]; - let cookies: Result, _> = names.into_iter().map(|name| _conn.intern_atom(false, name)).collect(); + let cookies: ::std::result::Result<::std::vec::Vec<_>, _> + = names.into_iter().map(|name| _conn.intern_atom(false, name)).collect(); Ok($cookie_name { - __private_phantom: std::marker::PhantomData, + __private_phantom: ::std::marker::PhantomData, __private_cookies: cookies?, }) } diff --git a/x11rb/tests/x11_utils.rs b/x11rb/tests/x11_utils.rs index fbd47e3e..f78a21af 100644 --- a/x11rb/tests/x11_utils.rs +++ b/x11rb/tests/x11_utils.rs @@ -1,5 +1,8 @@ -use std::{collections::HashMap, io::IoSlice}; -use x11rb::{ +use ::std::{ + collections::HashMap, io::IoSlice, option::Option as RealOption, result::Result as RealResult, + vec::Vec as RealVec, +}; +use ::x11rb::{ atom_manager, connection::{ BufWithFds, DiscardMode, ReplyOrError, RequestConnection, RequestKind, SequenceNumber, @@ -11,6 +14,19 @@ use x11rb::{ x11_utils::{ExtensionInformation, Serialize, TryParse, TryParseFd}, }; +// Just to be annoying, overwrite some standard types. +// We want to make sure atom_manager! does not use these directly. +mod std {} +mod x11rb {} +#[allow(dead_code)] +type Vec = (); +#[allow(dead_code)] +type Result = (); +#[allow(dead_code)] +type PhantomData = (); +#[allow(dead_code)] +type Option = (); + atom_manager! { Atoms: AtomsCookies { FIRST, @@ -24,21 +40,21 @@ struct AtomFakeConnection { // Mapping from byte string to the corresponding atom value and sequence number. // If no entry is found, sending the InternAtom request fails. // If the entry does not fit into u16, fetching the reply fails. - atoms_and_cookies: HashMap, u32>, + atoms_and_cookies: HashMap, u32>, } impl RequestConnection for AtomFakeConnection { - type Buf = Vec; + type Buf = RealVec; fn send_request_with_reply( &self, bufs: &[IoSlice], - _fds: Vec, - ) -> Result, ConnectionError> + _fds: RealVec, + ) -> RealResult, ConnectionError> where R: TryParse, { - let bytes: Vec = bufs.iter().flat_map(|buf| buf.iter().copied()).collect(); + let bytes: RealVec = bufs.iter().flat_map(|buf| buf.iter().copied()).collect(); // request type and only-if-exists flag assert_eq!(bytes[..2], [INTERN_ATOM_REQUEST, 0]); // We ignore the length field @@ -55,8 +71,8 @@ impl RequestConnection for AtomFakeConnection { fn send_request_with_reply_with_fds( &self, _bufs: &[IoSlice], - _fds: Vec, - ) -> Result, ConnectionError> + _fds: RealVec, + ) -> RealResult, ConnectionError> where R: TryParseFd, { @@ -66,8 +82,8 @@ impl RequestConnection for AtomFakeConnection { fn send_request_without_reply( &self, _bufs: &[IoSlice], - _fds: Vec, - ) -> Result, ConnectionError> { + _fds: RealVec, + ) -> RealResult, ConnectionError> { unimplemented!() } @@ -78,21 +94,21 @@ impl RequestConnection for AtomFakeConnection { fn prefetch_extension_information( &self, _extension_name: &'static str, - ) -> Result<(), ConnectionError> { + ) -> RealResult<(), ConnectionError> { unimplemented!(); } fn extension_information( &self, _extension_name: &'static str, - ) -> Result, ConnectionError> { + ) -> RealResult, ConnectionError> { unimplemented!() } fn wait_for_reply_or_raw_error( &self, sequence: SequenceNumber, - ) -> Result>, ConnectionError> { + ) -> RealResult>, ConnectionError> { let sequence_u16 = match u16::try_from(sequence) { Ok(value) => value, Err(_) => return Err(ConnectionError::UnsupportedExtension), @@ -110,21 +126,21 @@ impl RequestConnection for AtomFakeConnection { fn wait_for_reply( &self, _sequence: SequenceNumber, - ) -> Result>, ConnectionError> { + ) -> RealResult>, ConnectionError> { unimplemented!() } fn wait_for_reply_with_fds_raw( &self, _sequence: SequenceNumber, - ) -> Result>, Vec>, ConnectionError> { + ) -> RealResult>, RealVec>, ConnectionError> { unimplemented!() } fn check_for_raw_error( &self, _sequence: SequenceNumber, - ) -> Result>, ConnectionError> { + ) -> RealResult>, ConnectionError> { unimplemented!() } @@ -136,11 +152,11 @@ impl RequestConnection for AtomFakeConnection { unimplemented!() } - fn parse_error(&self, _error: &[u8]) -> Result { + fn parse_error(&self, _error: &[u8]) -> RealResult<::x11rb::x11_utils::X11Error, ParseError> { unimplemented!() } - fn parse_event(&self, _event: &[u8]) -> Result { + fn parse_event(&self, _event: &[u8]) -> RealResult<::x11rb::protocol::Event, ParseError> { unimplemented!() } }