Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mqtt payload for publish message copied 2 times during lifetime #13844

Open
doom369 opened this issue Feb 14, 2024 · 4 comments · May be fixed by #13939
Open

Mqtt payload for publish message copied 2 times during lifetime #13844

doom369 opened this issue Feb 14, 2024 · 4 comments · May be fixed by #13939

Comments

@doom369
Copy link
Contributor

doom369 commented Feb 14, 2024

PublishBuilder.build():

        public MqttPublishMessage build() {
            MqttFixedHeader mqttFixedHeader = new MqttFixedHeader(MqttMessageType.PUBLISH, false, qos, retained, 0);
            MqttPublishVariableHeader mqttVariableHeader =
                    new MqttPublishVariableHeader(topic, messageId, mqttProperties);
            return new MqttPublishMessage(mqttFixedHeader, mqttVariableHeader, Unpooled.buffer().writeBytes(payload));
        }

has Unpooled.buffer().writeBytes(payload). This is one is not big deal as we can easily overcome it.

MqttEncoder.encodePublishMessage:

ByteBuf payload = message.payload().duplicate();

My understanding, the authors were playing safe, however 2 copies, seems like overkill. WDYT? Should I fix it?

@normanmaurer
Copy link
Member

@doom369 sounds like something we can change .. Do you want to do a PR ?

I think it should be message.payload().retainedDuplicate()

@doom369
Copy link
Contributor Author

doom369 commented Feb 14, 2024

@normanmaurer I was thinking about:

message.payload().duplicate() -> message.payload()

In a typical use case you would do:

        ByteBuf buffer = ctx.alloc().buffer(body.length());
        buffer.writeCharSequence(body, StandardCharsets.UTF_8);

        MqttPublishMessage message = MqttMessageBuilders.publish()
                .payload(buffer)
                .qos(MqttQoS.AT_LEAST_ONCE)
                .messageId(packetId)
                .build();
       ctx.write(message);

Do you think this duplicate() has some purpose?

@franz1981
Copy link
Contributor

franz1981 commented Feb 14, 2024

I would allocate an ioBuffer and perform the copy into it, which will still happen later in the transport layers...
I am not a big fan of copies eh...we could have a builder which allow to pass some arbitrary buffer too, and will allow users to allocate it themselves, so we can enable some zero-copy semantic, if is a direct one.

@doom369
Copy link
Contributor Author

doom369 commented Feb 14, 2024

we could have a builder which allow to pass some arbitrary buffer too, and will allow users to allocate it themselves

That's what the current MqttMessageBuilders has already. The question - why the encoder does duplicate(), doesn't seem necessary for me.

Also, from the netty-end-user point of view - buffers are not necessary, I just want to send a string message back into mqtt channel (most common use case). So for me, it would be more convenient to send String instead of ByteBuf. But that's another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants