Use new type Estring to avoid cloning &'static str

[emilk]

ui.label("static string") is a very common use case, and currently egui clones the string in these cases.

This PR introduces a new type:

pub enum Estring {
    Static(&'static str),
    Owned(Arc<str>),
}

which is used everywhere text is needed, with impl Into<Estring> in the API for e.g. ui.label.

String and &'static str both implement Into<Estring>, but non-'static &str does not (since I don't think there is any way to do so without lifetime specialization, which rust lacks).

The good

This reduces the number of copies drastically and speeds up the benchmark demo_with_tessellate__realistic by 17%.

The bad

This hurts the ergonomics of egui a bit, and this is a breaking change.

For instance, this used to work:

fn my_label(ui: &mut egui::Ui, text: &str) {
    ui.label(text);
}

This must now either be changed to

fn my_label(ui: &mut egui::Ui, text: &str) {
    ui.label(text.to_string());
}

or the argument must be changed to either text: &'static str or text: String.

The ugly

The arguments for various egui functions also become less clear. When the argument is impl ToString like now, it is clear to most people what is acceptable (and most things are acceptable, too). When this changes to impl Into<Estring> it requires people reading up on Estring and understanding that String and &'static str implements Into<Estring>, but &str does not. Basically, a worse API.

Summary

A nice speedup, but at the cost of a breaking change and worse ergonomics. I'm not quite sure how I feel about this.

Please help!

I would apreciate feedback about this change. How important is performance to you? How important is ergonomics?

You can try out this branch by adding the following to the end of your Cargo.toml:

[patch.crates-io]
egui = { git = "https://github.com/emilk/egui", branch = "estring" }

then report back how much or little your code breaks, and if you think the performance benefits are worth it!

Alternatives

The reason Estring must be 'static is that egui caches the results of string layout, and the string is part of the key (and thus stored from one frame to the next). So it must either be owning (String, Arc<str>, …) or borrowing a &'static str.

This cache key could be changed to only storing the hash of the input string (and hope there are no collisions). Then we could use impl AsRef<str> as the generic string argument, which would be a lot more ergonomic, as it would allow all &str, not just &'static str. It would add a lot of <'a> to a lot of egui code, which is a bit ugly, but maybe worth it.

I'll try exploring this alternative when I find the time.

[aloucks]

I don't think the ergonomic hit is worth it. Is egui the bottleneck for anything? I'm also curious if the performance increase due to reduced allocations or from raw copying of the strings. Perhaps there are other optimizations that could be applied without the hit to ergonomics?

This cache key could be changed to only storing the hash of the input string (and hope there are no collisions). Then we could use impl AsRef as the generic string argument, which would be a lot more ergonomic, as it would allow all &str, not just &'static str.

Although it's been a while since I looked at the imgui code, I want to say that it does something similar and uses a rather weak hash algorithm (I think it was crc32).

[aloucks]

Have you considered string interning as an alternative? https://matklad.github.io/2020/03/22/fast-simple-rust-interner.html

[emilk]

Have you considered string interning as an alternative? https://matklad.github.io/2020/03/22/fast-simple-rust-interner.html

String interning is an interesting idea to explore. It would replace the cost of a clone with the cost of a hash and hashmap lookup (followed by a clone in the dynamic cases), so hard to predict if it will help.

[emilk]

I think the ergonomics of egui is one of its main selling points, so let us not compromise it by this much.

[aloucks]

This may be useful as an internal representation of strings to help reduce allocation:
https://github.com/ParkMyCar/compact_str

[Mingun]

Actually, it would be very useful, if ui.label("static string") constructs could be forbidden on type level, so that the developer couldn't show anything in UI that is not localizable. That is required a special type for localizable strings.