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

Store outgoing_pub in a HashMap for more memory efficient storage of unassigned pkids #880

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ambaxter
Copy link

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

I wrote an MQTT traffic simulator and noticed that creating many clients within a single process consumes a large amount of memory. The below example is for 1000 clients.

mqtt_before

This is due to rumqttc pre-allocating MqttState.outgoing_pub for each client. I replaced the Vec with a HashMap and dramatically reduced memory usage.

mqtt_after

I know that MqttState.outgoing_pub is most likely pre-allocated for low powered devices. Would it be possible to make using a HashMap either a runtime configuration or feature flag?

@de-sh
Copy link
Member

de-sh commented Jun 17, 2024

By any chance does the code with which you tested these scenarios use QoS 1+?

Seems like the HashMap is saving space as it never gets utilized(which is the case in QoS 0). That shouldn't be the case when we end up using QoS 1+ where allocations will take up some cycles as the map size increases slowly.

@swanandx
Copy link
Member

you can also configure how much you want to preallocate using MqttOptions, let's say you set max_inflight to be 10 to save memory, it won't allocate more than that.

@ambaxter
Copy link
Author

By any chance does the code with which you tested use QoS 1+?

Seems like the HashMap is saving space as it never gets utilized(which is the case in QoS 0). That shouldn't be the case when we end up using QoS 1+ where allocations will take up some cycles as the map size increases slowly.

The simulation I've developed only uses QoS 1 &2. The HashMap should only be as large as the number of Publish entries waiting on PubRec or PubAck. Each HashMap could very well grow to be u16::MAX in length.

My test-case is currently running at 10,000 MQTT clients at ~1500 messages/s for the entire simulation. The simulator's RAM hasn't breached 800 MB in almost an hour of processing.

@ambaxter
Copy link
Author

you can also configure how much you want to preallocate using MqttOptions, let's say you set max_inflight to be 10 to save memory, it won't allocate more than that.

True, but in many cases you won't know what the correct value should be just based off of traffic size alone. There are many variables to consider.

@xiaocq2001
Copy link
Contributor

IMHO we can use VecDeque to save pkid/publish and NoticeTx, the reasons:

  1. Since the packets are always ordered, the push_back and pop_front can be used, they work faster than insert and push of Vec.
  2. The memory usage is less than HashMap

Some of the proposed changes are included here:
68fcb73#diff-35dca8779a1d08a58f298e7f88a1ca9d2c32f79e97024252a6366e4a2f32325a

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 this pull request may close these issues.

4 participants