Optimise atom_manager! implementation #914

psychon · 2024-02-03T12:57:01Z

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<Item=Result<Cookie, Error>> into Result<Vec, 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:
rust-lang/reference#888

Fixes: #912

I tried to measure this with latest winit master:

$ cargo build --release --no-default-features --features x11 --example video_modes
$ ls -lh target/release/examples/video_modes
$ strip target/release/examples/video_modes
$ ls -lh target/release/examples/video_modes

With the latest x11rb release, the result is 16 MB / 2,0 MB. With this change (copy&paste the macro into the winit code), it is 15 MB / 2.0 MB.

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<Item=Result<Cookie, Error>> into Result<Vec<Cookie>, 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: rust-lang/reference#888 Fixes: #912 Signed-off-by: Uli Schlachter <psychon@znc.in>

codecov · 2024-02-03T12:59:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0fbfd89) 13.06% compared to head (39be186) 13.07%.
Report is 2 commits behind head on master.

❗ Current head 39be186 differs from pull request most recent head f431b67. Consider uploading reports for the commit f431b67 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #914   +/-   ##
=======================================
  Coverage   13.06%   13.07%           
=======================================
  Files         190      190           
  Lines      136667   136674    +7     
=======================================
+ Hits        17859    17870   +11     
+ Misses     118808   118804    -4

Flag	Coverage Δ
tests	`13.07% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

notgull

LGTM aside from some macro hygeine

notgull · 2024-02-04T18:09:14Z

x11rb/src/x11_utils.rs

-                $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>>,


Suggested change

__private_cookies: Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>,

__private_cookies: ::std::vec::Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>,

Just in case a potential user defines their own Vec struct.

Thanks for catching that! I turned the existing tests/x11_utils.rs into a test case for that and also found a std:: which should be ::std::.

notgull · 2024-02-04T18:09:41Z

x11rb/src/x11_utils.rs

+                let names = [
+                    $($crate::__atom_manager_atom_value!($field_name$(: $atom_value)?),)*
+                ];
+                let cookies: Result<Vec<_>, _> = names.into_iter().map(|name| _conn.intern_atom(false, name)).collect();


Suggested change

let cookies: Result<Vec<_>, _> = 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();

notgull · 2024-02-04T18:10:49Z

x11rb/src/x11_utils.rs

+                let names = [
+                    $($crate::__atom_manager_atom_value!($field_name$(: $atom_value)?),)*
+                ];
+                let cookies: Result<Vec<_>, _> = names.into_iter().map(|name| _conn.intern_atom(false, name)).collect();


_conn.intern_atom(

Should we import the x11rb::protocol::xproto::ConnectionExt trait here, in case the user hasn't already?

Uhm... this doesn't need ConnectionExt?

I wanted to make x11rb/tests/x11_utils.rs test for this, but it already does. It does not import ConnectionExt.

If I had to guess, I would say the bound C: $crate::protocol::xproto::ConnectionExt brings the trait into scope.

yshui · 2024-02-05T02:22:54Z

Interesting find! I wonder if rustc is being particularly dumb here or if, say, g++ has the same behaviour 🤔

psychon · 2024-02-05T15:07:15Z

@yshui FYI, I reported this upstream: rust-lang/rust#120604
Dunno how I could test something precisely like this with G++. There is not really an equivalent for ? in C++ and stuff is quite different in general.

Here is an attempt.

Link to godbolt

#include <optional>

struct Dropper {
    Dropper();
    ~Dropper();
};

bool choice();

struct Drops {
    Dropper a, b;
#ifdef DO_C
    Dropper c;
#endif
#ifdef DO_D
    Dropper d;
#endif
};

std::optional<Drops> __attribute__((noinline)) do_it() {
    Dropper a;
    if (choice()) { return std::nullopt; }
    Dropper b;
    if (choice()) { return std::nullopt; }
#ifdef DO_C
    Dropper c;
    if (choice()) { return std::nullopt; }
#endif
    return Drops {
        .a = std::move(a),
        .b = std::move(b),
#ifdef DO_C
        .c = std::move(c),
#endif
#ifdef DO_D
        .d = std::move(d),
#endif
    };
}

Without #defines, there are four calls to ~Dropper in do_it. With DO_C, there are six calls. DO_C plus DO_D produces eight calls.

That seems linear to me.

@notgull

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 <psychon@znc.in>

yshui · 2024-02-05T17:08:42Z

@psychon slightly tweaked your example so it's a bit clear: https://godbolt.org/z/GqWcPn9bs

so g++ indeed does what i expect here, it's essentially:

    Dropper a;
    if (cond1) goto free1;
    Dropper b;
    if (cond2) goto free2;
    Dropper c;
    if (cond3) goto free3;

    //....

free3:
    delete c;
free2:
    delete b;
free1:
    delete a;
    return

definitely something worth fixing in Rust. I am not familiar with rustc though....

yshui · 2024-02-05T17:11:16Z

ah but in Rust's case it's more complex because some variables could have been moved thus don't need dropping.

psychon mentioned this pull request Feb 4, 2024

Quadratic code size due to combination of ? and Drop for linear code rust-lang/rust#120604

Open

notgull requested changes Feb 4, 2024

View reviewed changes

psychon requested a review from notgull February 15, 2024 20:22

notgull approved these changes Feb 15, 2024

View reviewed changes

mergify bot merged commit 6592b38 into master Feb 15, 2024
21 checks passed

mergify bot deleted the atom-manager-binary-size branch February 15, 2024 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise atom_manager! implementation #914

Optimise atom_manager! implementation #914

psychon commented Feb 3, 2024

codecov bot commented Feb 3, 2024 •

edited

Loading

notgull left a comment

notgull Feb 4, 2024

psychon Feb 5, 2024

notgull Feb 4, 2024

notgull Feb 4, 2024

psychon Feb 5, 2024

yshui commented Feb 5, 2024

psychon commented Feb 5, 2024

yshui commented Feb 5, 2024

yshui commented Feb 5, 2024

	__private_cookies: Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>,
	__private_cookies: ::std::vec::Vec<$crate::cookie::Cookie<'a, C, $crate::protocol::xproto::InternAtomReply>>,

	let cookies: Result<Vec<_>, _> = 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();

Optimise atom_manager! implementation #914

Optimise atom_manager! implementation #914

Conversation

psychon commented Feb 3, 2024

codecov bot commented Feb 3, 2024 • edited Loading

Codecov Report

notgull left a comment

Choose a reason for hiding this comment

notgull Feb 4, 2024

Choose a reason for hiding this comment

psychon Feb 5, 2024

Choose a reason for hiding this comment

notgull Feb 4, 2024

Choose a reason for hiding this comment

notgull Feb 4, 2024

Choose a reason for hiding this comment

psychon Feb 5, 2024

Choose a reason for hiding this comment

yshui commented Feb 5, 2024

psychon commented Feb 5, 2024

yshui commented Feb 5, 2024

yshui commented Feb 5, 2024

codecov bot commented Feb 3, 2024 •

edited

Loading