Stream: beginners

Topic: Feedback to MQTT Client PoC

Jonas Diemer (Mar 29 2025 at 18:19):

Hi,

I wrote a first basic version of an MQTT client. It can connect to a server and subscribe to topics and receive their messages.

https://gist.github.com/jonasdiemer/6b650f473e990bff5f8d216bb3d9b371

I'd be interested in feedback as to what could be improved, especially related to "idiomatic Roc". Thanks in advance!

Brendan Hansknecht (Mar 29 2025 at 18:21):

bug on line 11, I think you want:
Num.to_u8(Num.shift_right_by(l, 8)) (with to_u8 as the outer function call)

Brendan Hansknecht (Mar 29 2025 at 18:24):

Str.count_utf8_bytes(payloadStr) and List.len(Str.to_utf8(payloadStr)) are the same thing. So you only need to grab one of them.

I would probably do:

addPayload = |payload, payloadStr|
    if !Str.is_empty(payloadStr) then
        bytes = Str.to_utf8(payloadStr)
        l = List.len bytes
        payload
            |> List.append(Num.to_u8(Num.shift_right_by(l, 8)))
            |> List.append(Num.to_u8(Num.bitwise_and(l, 0xFF)))
            |> List.concat(bytes)
    else
        payload

Brendan Hansknecht (Mar 29 2025 at 18:27):

I wouldn't advise using default value record fields { clientIdStr, userName ?? "", password ?? "" }. They have known bugs and I believe are slotted for removal currently.

Brendan Hansknecht (Mar 29 2025 at 18:27):

Instead you can use a result or tag union of some sort

Brendan Hansknecht (Mar 29 2025 at 18:29):

Pretty minor, but all of these little lists connectFlags = [connectFlagsValue] allocate. I would avoid that and avoid List.join if possible. Instead, I would focus on using List.append and List.concat

Brendan Hansknecht (Mar 29 2025 at 18:34):

As a direct example, line 41 will lead to a new allocation and copying all the data, which is quite expensive. Would be better to either precalculate the length or to initially set to to zero. Then go back and List.set it to the value you want. Overall, this has a ton of extra copies and allocation. Not sure if you care, but important if you want to use best practices for perf.

Brendan Hansknecht (Mar 29 2025 at 18:40):

Totally optional, but for all of the list reading after exact byte reading from tcp, I would probably transform the error. Either just crash cause it should be unreachable or give it a better error than the default List.get would give you. I would just make it a function likeexactListGet or something. Also, hopefully the rust side has buffer, but loading and reading just 1 or 2 bytes at a time, likely will hurt perf a good bit. This is were roc could really use a better solution, like theoretically could return a tuple, but they you need a special method for each tuple size.

Brendan Hansknecht (Mar 29 2025 at 18:41):

At like 130, I would always try to avoid using Bool.true. Instead, just write the equality and avoid the when is.ackFlags == 0x01

Brendan Hansknecht (Mar 29 2025 at 18:42):

Overall looks quite reasonable for bit twiddling code in roc

Brendan Hansknecht (Mar 29 2025 at 18:42):

Mostly makes me want to have inline bitwise operators in roc (I still am in the camp that we really should add these especially if we expect many libraries to be put in roc directly)

Anton (Mar 29 2025 at 18:47):

inline bitwise operators in roc

This code could be a nice sample to compare the current version vs with bitwise operators.

Jonas Diemer (Mar 29 2025 at 19:09):

Brendan Hansknecht said:

bug on line 11, I think you want:
Num.to_u8(Num.shift_right_by(l, 8)) (with to_u8 as the outer function call)

Thanks for the reply. On line 11, there's a Num.to_u8 in the second element of the list - wouldn't that force the first one to be of u8 too?

Brendan Hansknecht (Mar 29 2025 at 19:11):

The original code Num.shift_right_by(Num.to_u8(l), 8) always returns zero. Cause converting l to u8 will return a value between 0 and 255, truncating l.

Brendan Hansknecht (Mar 29 2025 at 19:16):

or, it will actually always return 0 or 255 depending on the value of the first bit (might be undefined behaviour with optimizations)

Brendan Hansknecht (Mar 29 2025 at 19:20):

» Num.shift_right_by(Num.to_u8(0xFF10), 8)

0 : U8
» Num.shift_right_by(Num.to_u8(0xFF80), 8)

255 : U8
» Num.shift_right_by(Num.to_u8(0x0080), 8)

255 : U8

vs:

» Num.to_u8(Num.shift_right_by(0xFF10, 8))

255 : U8
» Num.to_u8(Num.shift_right_by(0xFF80, 8))

255 : U8
» Num.to_u8(Num.shift_right_by(0x0080, 8))

0 : U8

Jonas Diemer (Mar 29 2025 at 19:24):

Brendan Hansknecht said:

As a direct example, line 41 will lead to a new allocation and copying all the data, which is quite expensive. Would be better to either precalculate the length or to initially set to to zero. Then go back and List.set it to the value you want. Overall, this has a ton of extra copies and allocation. Not sure if you care, but important if you want to use best practices for perf.

So basically replace line 41 with something like this?

fixedHeader
    |> List.concat(protocolName)
    |> List.concat(protocolLevel)
    |> List.concat(connectFlags)
    |> List.concat(keepAliveBytes)
    |> List.concat(payload)

And of course removing the other join()s above. Would concat() of small lists like connectFlags still be overhead vs. using append() of the value?

Brendan Hansknecht (Mar 29 2025 at 19:26):

Join and concat of small lists are the same. They allocate all the intermediate lists. I would only use it for pre-existing strings/lists if possible and use append otherwise.

Brendan Hansknecht (Mar 29 2025 at 19:27):

The issue is that every small list is an allocation and then a copy

Brendan Hansknecht (Mar 29 2025 at 19:27):

Roc has no sort of small list optimization or stack allocated array

Last updated: Jul 06 2025 at 12:14 UTC