Change type of APS sound/category to Cow<'static, str>

[dbrgn]

This way a static sound and category can be specified, thus avoiding an allocation for every notification.

The API is still ergonomic, thanks to Into<Cow<'static, str>>.

A question would be whether to use custom lifetimes instead of 'static, but then those lifetimes would propagate through the entire type hierarchy. Not sure if you want that.

[dbrgn]

Sorry for the force push, I added some more lines to the tests to cover 'static and owned strings too.

[pimeys]

I would think about this a bit though before merging.

Now a push from Europe to APNs will take about 150-300ms. Allocating a couple of strings for every push will not take that long in comparison when we use jemalloc anyways. I'd like to actually see how this makes the code faster before going with a quite ugly type signature of Option<Cow<'static, str>>. I'll try your changes and maybe extending them to the other Strings in the notifications, to see how the API looks like.

We use about 3 cores and less than 100 megabytes of RAM for our multi-app setup, so this would maybe save a bit of CPU time and some bytes of RAM...

[dbrgn]

The good thing is that for the users the API does not change. They can still call it with a String or a &str. The "ugliness" is only internally.

To make the code a bit nicer internally you could use type aliases:

type StringLike = Cow<'static, str>;
type IntoStringLike = Into<StringLike>;

pub fn set_sound<S>(&mut self, sound: S) where S: IntoStringLike {
    self.sound = Some(sound.into());
}

[pimeys]

I was thinking more that could we actually get rid of String everywhere in the hot parts? I'd use something else than a static lifetime though, bind the payload items with a lifetime parameter. I'll try it out, just fetched your branch.

[dbrgn]

If you don't actually need owned strings, then passing in &str everyhwere would be quite nice. It does have the cosmetic downside of many lifetime parameters though.

[pimeys]

And the other thing is I've never seen Cow used like that with a static lifetime. So how does it work with a heap-allocated String, where does it get its lifetime, or will it work at all?

[dbrgn]

Cow<'static, str> only works with static references, or with owned strings. This is better than the status quo of only accepting owned strings, but of course it does not work with references to heap-allocated values.

On the other hand if we'd think about accepting Cow<'a, str> then we could just as well parametrize the entire struct and just use &'a str instead. Using Cow<'static, str> was simply a way of accepting static strings without adding any new lifetimes.

[pimeys]

I did a try to just use &'a str instead of String everywhere in the payloads, but the builder pattern here really makes it hard. Let's get back to the Cow and try to use it with 'a instead of 'static, so we get the biggest possible amount of types we can use in here.

[dbrgn]

That won't change anything about the API though. If you use 'a in the Cow, then that needs to be defined on your structs too, just as if you would use &'a str.

The only advantage of a Cow<'a, str> over a &'a str is that the user can pass in an owned String, instead of passing in a reference to their string that might get cloned afterwards.

Ideally we'd accept &'a str and would never actually clone the string of course. But I'm not sure if that's possible with the current futures based API.

[pimeys]

Ok, so I tried with 'a str and it gets overly complex. But I'm not merging this yet before actually seeing this as a performance culprit. It's not very nice if we now block people using dynamically allocated strings for category and sound.

What I'd like to see is usage of &'a str everywhere in payloads. I've tried that a couple of times, and decided the change is too complex for the gains we get.

[dbrgn]

If you want I can also give it a try. I think it should be doable, as long as you don't mind the lifetime parameters.

[pimeys]

Go for it if you have time! It's a good place to learn some harder Rust lifetime tricks. You might need PhantomData and maybe need to change some of the builders so the lifetimes match. Try to get rid of all Strings! :)

[pimeys]

I will close this in favour of another approach. Please reopen if needed.

https://github.com/pimeys/apns2/pull/10