Experiment: Borrowing payload

[dbrgn]

As discussed in #8, this is a first attempt at having less owned data in the payload.

@pimeys right now this only considers a single field (the device token). If you agree with the general approach I can try to expand this to other strings.

[pimeys]

I agree with this path. This is a bit different from my approach of using Into<&'a str>, but I think your approach might work better.

The hard parts will be with the builders and keeping the lifetimes working. Lots of tedious work, so you should also consider how big of a win this in performance-wise.

I like the builder pattern in the payload generation. It tells strict boundaries for different types of notifications, but can be rough when you take the ownership out from the contents. Would be also nice to understand how does the serde serialization work when the serialized structs borrow their content. If I remember right it should not allocate in these cases, just gives a lens to the original data in JSON form.

Summarizing, go with a minimal change first, start from the localized notification, change one field from String to &'a str in the builder and the actual structure and try to get the build to work.

[pimeys]

Oh, one more thing to consider with the lifetimes... When we have a Payload<'a> bound to references from outside data, how will the lifetimes work when you generate a future with the client and send it to a Core. Even if you get this to compile, it would be nice to test it with a simple consumer that gets messages somewhere and sends push notifications with a Core. My worry is are we blocking some way of using this library with a more restrictive ownership policy.

[dbrgn]

how will the lifetimes work when you generate a future with the client and send it to a Core

Yep, that's actually a good point... :+1:

[dbrgn]

Hm, like you probably expected, I underestimated how tricky putting borrows everywhere is, because the first change (870325b2835d26b94611e523d5f36223b5d1277c) was so easy :)

So instead of borrowing everything, maybe we can find a middle ground that gives some improvements with low effort?

This is the Payload:

pub struct Payload<'a> {
    pub options: NotificationOptions,
    pub device_token: &'a str,
    pub aps: APS,
    pub custom_data: Option<HashMap<String, Value>>,
}

pub struct NotificationOptions {
    pub apns_id: Option<String>,
    pub apns_expiration: Option<u64>,
    pub apns_priority: Priority,
    pub apns_topic: Option<String>,
    pub apns_collapse_id: Option<CollapseId>,
}

pub struct APS {
    pub alert: Option<APSAlert>,
    pub badge: Option<u32>,
    pub sound: Option<String>,
    pub content_available: Option<u8>,
    pub category: Option<String>,
    pub mutable_content: Option<u8>,
}

Maybe instead of borrowing the values inside the notification options, we can borrow the entire struct?

I pushed a commit that borrows the NotificationOption structs in the payload.

The APS would be more difficult, since the struct itself is constructed in the builders.

[pimeys]

This task is not trivial, I know. It will affect the throughput a bit if you don't need to allocate from heap in payloads, but we've witnessed some thousands concurrent notifications in one pipeline during the 150-300ms average response time, so when measuring performance, it needs to be done by counting the throughput.

[pimeys]

Ok, I went through the same idea when I was refactoring a2 on this weekend. The biggest problem is our Client, especially how it implements Service:

The Payload is here the Service implementation's Request. The NotificationOptions and device_token belong to Payload, so setting the lifetime variables there will require us to define type Request = Payload<'a>, which makes us to write impl<'a> Service for Client<'a>, therefore when we have a consumer, and we want to store an instance of Client<'a>, we must bubble the lifetime variable to the consumer struct too, and of course Client<'a> must hold PhantomData<'a ...> so the lifetime variable causes way too much trouble.

Here I'm kind of against this refactoring. The lifetimes bubble up just because tokio-service forces us to define our Request type.

[dbrgn]

Ok, maybe we should simply accept allocations where data is dynamic, and offer the Cow<'static, str> approach as an alternative if the data is static? If you worry about ugly type signatures you can create a type alias for it.

[pimeys]

Closing this, kind of fixed in another ticket:

https://github.com/pimeys/a2/pull/25