yield by each x delivered bytes instead of each y msg/frame

[viktorerlingsson]

WHAT is this pull request doing?

Changes read_loop and deliver_loop to Fiber.yield based on delivered bytes instead of number of messages/frames. This should make it so LavinMQ does not freeze when handling large volumes of large messages.

Performance wise it seems pretty similar to main with small messages, and much more stable with larger messages.

this branch

lavinmqperf throughput
Average publish rate: 802665.3 msgs/s
Average consume rate: 798787.9 msgs/s

lavinmqperf throughput -s 100000
Average publish rate: 7346.7 msgs/s
Average consume rate: 5862.9 msgs/s

lavinmqperf throughput -s 1000000
Average publish rate: 738.6 msgs/s
Average consume rate: 738.3 msgs/s

main

lavinmqperf throughput
Average publish rate: 792352.1 msgs/s
Average consume rate: 793866.8 msgs/s

lavinmqperf throughput -s 100000
Average publish rate: 6883.7 msgs/s
Average consume rate: 131.7 msgs/s

lavinmqperf throughput -s 1000000
Average publish rate: 542.9 msgs/s
Average consume rate: 4.3 msgs/s

HOW can this pull request be tested?

Existing specs should cover this pretty well.

[carlhoerberg]

Or skip my suggested type change, it would only become a problem if someone sent a frame larger than 2 GiB.. (which a rouge client potentially could do, but then, we don't do wrap around checks so nothing would even crash)

[carlhoerberg]

Trying to write good docs for these settings makes me think that this probably shouldn't be user configurable settings at all. We only want it to be configurable to be able to easy test out new settings internally. The long term solution is either a fixed value, not something we expect users to change, or better yet, waiting for the better MT implementation in crystal that force "yield" fibers after 10ms or something.

[viktorerlingsson]

Trying to write good docs for these settings makes me think that this probably shouldn't be user configurable settings at all. We only want it to be configurable to be able to easy test out new settings internally. The long term solution is either a fixed value, not something we expect users to change, or better yet, waiting for the better MT implementation in crystal that force "yield" fibers after 10ms or something.

Yeah, agreed for the most part. In most cases these values shouldn't need to be changed. There might be a case for changing them in some extreme cases, but it can also cause some "harm" if users set the values without understanding them.

@spuun since you suggested making the values configurable, WDYT?

[spuun]

Trying to write good docs for these settings makes me think that this probably shouldn't be user configurable settings at all. We only want it to be configurable to be able to easy test out new settings internally. The long term solution is either a fixed value, not something we expect users to change, or better yet, waiting for the better MT implementation in crystal that force "yield" fibers after 10ms or something.

Yeah, agreed for the most part. In most cases these values shouldn't need to be changed. There might be a case for changing them in some extreme cases, but it can also cause some "harm" if users set the values without understanding them.

@spuun since you suggested making the values configurable, WDYT?

Yeah, hm. I think it could be nice to have them until MT to be able to tweak individual instances. "Hide" them as ENVs instead? Or just document? Put them in an [experimental] section?

I think it would be nice to avoid having to do releases just to tweak values such this even more.

[carlhoerberg]

"experimental" section sounds good

…

On Thu, Dec 5, 2024 at 3:01 PM Jon Börjesson ***@***.***> wrote: Trying to write good docs for these settings makes me think that this probably shouldn't be user configurable settings at all. We only want it to be configurable to be able to easy test out new settings internally. The long term solution is either a fixed value, not something we expect users to change, or better yet, waiting for the better MT implementation in crystal that force "yield" fibers after 10ms or something. Yeah, agreed for the most part. In most cases these values shouldn't need to be changed. There might be a case for changing them in some extreme cases, but it can also cause some "harm" if users set the values without understanding them. @spuun <https://github.com/spuun> since you suggested making the values configurable, WDYT? Yeah, hm. I think it could be nice to have them until MT to be able to tweak individual instances. "Hide" them as ENVs instead? Or just document? Put them in an [experimental] section? I think it would be nice to avoid having to do releases just to tweak values such this even more. — Reply to this email directly, view it on GitHub <#863 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABL6TQKH3MPOLA67ORADOL2EBMCNAVCNFSM6AAAAABSS7P3H2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRQGQYDQMJZHE> . You are receiving this because your review was requested.Message ID: ***@***.***>