A performanсe when publishing many packets to the same topic

[YuriK75]

Hi!
As I see, for publish a packet from client - I must:

1. Make a topic String (or string slice) - contains own copy of data in heap
2. Make a TopicName which "eats" topic String (or copies string to own copy in heap) and checks it with RegExp
3. Make a payload - also own copy in heap
4. Make a PublishPacket which "eats" TopicName and payload
5. Serialize the packet to stream, which does not "eat" packet
6. Drop the packet

When I need (and many others probaly too) to publish different (millions by day) payloads to the same (thousands) topics -
i can't reuse TopicName for doing 1-2 steps once and doing only 3-6 steps for every publish.

Also i can't store a packet for later reuse - it does not support payload replacing.

So this architecture breaks performance.

If You can add ability for using in packet shared ref to TopicName (and for payload may be too, but it
is not important) - it will be great!

Or add convenience fn which receives &TopicName, payload: &[u8], &mut Vec
and serializes 1st and 2nd arguments to 3rd...

Yuri

[zonyitoo]

It seems that a PublishPacketRef will do the trick.

pub struct PublishPacketRef<'a> {
    fixed_header: FixedHeader,
    topic_name: &'a TopicNameRef,
    packet_identifier: Option<PacketIdentifier>,
    payload: &'a [u8],
}

And impls Packet for it. Very simple solution, but the Packet trait implies that the struct could be be Encodable and Decodable, but PublishPacketRef couldn't be Decodable.

[Spoonbender]

Hmmm perhaps split Packet to OutgoingPacket and IncomingPacket (Packet impls both), just so we can have this optimization of published packets?
The scenario @YuriK75 presented is quite common, and avoiding these allocations and deallocations would be great.

[YuriK75]

What about impl only Encodable for PublishPacketRef?
That PublishPacketRef will live very shortly - may be it is too complex for solving one small task...
May the only function will do all - at once will take all parameters and encode it to Write.

pub fn publish<W: Write>(
    // ... FixedHeader or any other
    topic_name: &TopicName,
    packet_identifier: Option<PacketIdentifier>,
    payload: &[u8],
    writer: &mut W) -> Result<(), PublishPacket::Err>

[zonyitoo]

Yes, I have already tried both @Spoonbender and @YuriK75 's proposal locally, yesterday.

1. Splits Packet into OutgoingPacket and IncomingPacket is good, but one problem have to be solved: how to define Pa cket::Payload? Or we should just remove payload() and payload_ref() member functions, and PacketError<T>'s definition of PayloadError, which are breaking changes.
2. impls Encodable for PublishPacketRef only is good, but what is the Err type? Normally it should be PacketError<T> as all the other *Packet types.

I haven't decided yet, maybe both of yours ideas should be adopted, which may lead to a huge API breaking chage.

[zonyitoo]

May the only function will do all - at once will take all parameters and encode it to Write.

Yes, that is a simpler solution, but only be useful for this case.

[Spoonbender]

@zonyitoo maybe a cow will help with the fields that are either owned or borrowed?

[zonyitoo]

Packet::decode_packet requires Self to have 'static lifetime.

[zonyitoo]

It should be ok to remove the Packet trait .. Hmm ... Very complicated.

[zonyitoo]

Please check if this commit works for you.

[YuriK75]

Please check if this commit works for you.

Hmmm... so many changes ... I think my issue much less difficult to solve...

1. Splitting Packet trait - ok, it is reasonable change!
2. New PublishPacketRef struct - at most ok.
3. New TopicNameRef tuple - wrong idea, completely unneeded type. I think in PublishPacketRef::topic_name You should use &TopicName instead of &TopicNameRef. And unsafe code in TopicNameRef::new() with dirty C-style pointers and unusing borrow checker feature - completely bad idea. Remove TopicNameRef at all, please!

Yuri

[zonyitoo]

TopicNameRef is not a new struct. But I think it is safe to remove because nobody is actually using it.

[YuriK75]

I forgot to say "thank you". You made a lot of work for me! пт, 26 февр. 2021 г. в 04:11, ty <notifications@github.com>:

…

TopicNameRef is not a new struct. But I think it is safe to remove because nobody is actually using it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOLFLPHLT6T2YP3CCCFKHO3TA3YN3ANCNFSM4XOQPHTA> .

[zonyitoo]

I just double checked the design of TopicNameRef. It is required because it is used in TopicFilterRef, which is "new type" for adding special methods for &str.

It is ok to left it there, you can just pass &TopicName as the first argument of PublishPacketRef::new.

[YuriK75]

As for me, PublishPacketRef should use &TopicName in topic_name field instead of &TopicNameRef.
It will be the only usable solution for me.

Can You change it?

I think TopicNameRef can be everywhere replaced by &TopicName (& is the same thing as "Ref").
TopicNameRef is some mistake struct... But You decide, may be i am wrong...

[zonyitoo]

TopicNameRef is for making a str type that is a VALID Topic Name.

mqtt-rs/src/topic_filter.rs

Lines 274 to 278 in e910579

	assert!(matcher.is_match(TopicNameRef::new("sport").unwrap()));
	assert!(matcher.is_match(TopicNameRef::new("/").unwrap()));
	assert!(matcher.is_match(TopicNameRef::new("abc/def").unwrap()));
	assert!(!matcher.is_match(TopicNameRef::new("$SYS").unwrap()));
	assert!(!matcher.is_match(TopicNameRef::new("$SYS/abc").unwrap()));

It will be the only usable solution for me.

Could you provide a code snippet about this? Is this design causing unfixable compile errors? Are there any uncomfortable use cases?