Enhancement: Get rid of unnecessary malloc/memcpy when processing publish message

[ralight]

migrated from Bugzilla #470258
status RESOLVED severity normal in component Mosquitto for 1.5
Reported in version unspecified on platform PC
Assigned to: Roger Light

On 2015-06-16 05:58:08 -0400, Yun Wu wrote:

mqtt3_handle_publish calls mqtt3_db_message_store.

In mqtt3_db_message_store, source_id, topic and payload are all newly-allocated and copied.

Since topic allocated in mqtt3_handle_publish is not used after mqtt3_db_message_store returned, we can get rid of once of unnecessary payload copy for better performance.

Here is my fix:

1. Introduce new function:

define F_NEW_SOURCE_ID (1 << 0)

define F_NEW_TOPIC (1 << 1)

define F_NEW_PAYLOAD (1 << 2)

int __mqtt3_db_message_store(int flags, struct mosquitto_db _db, const char *source_id, uint16_t source_mid, const char *topic, int qos, uint32_t payloadlen, const void *payload, int retain, struct mosquitto_msg_store *_stored, dbid_t store_id)
{
struct mosquitto_msg_store *temp = NULL;
char *new_source_id = NULL;
char *new_topic = NULL;
void *new_payload = NULL;

assert(db);
assert(stored);

if (flags & F_NEW_SOURCE_ID) {
new_source_id = _mosquitto_strdup(source_id ? source_id : "");
if(!new_source_id){
goto no_memory;
}
source_id = new_source_id;
}
if (flags & F_NEW_TOPIC) {
if (topic) {
new_topic = _mosquitto_strdup(topic);
if(!new_topic){
goto no_memory;
}
}
topic = new_topic;
}
if (flags & F_NEW_PAYLOAD) {
if (payloadlen && payload) {
new_payload = _mosquitto_malloc(sizeof(char)_payloadlen);
if(!new_payload){
goto no_memory;
}
memcpy(new_payload, payload, sizeof(char)_payloadlen);
}
payload = new_payload;
}

temp = _mosquitto_calloc(1, sizeof(struct mosquitto_msg_store));
if(!temp) goto no_memory;

temp->ref_count = 0;
temp->source_id = (char *)source_id;
temp->source_mid = source_mid;
temp->msg.mid = 0;
temp->msg.qos = qos;
temp->msg.retain = retain;
temp->msg.topic = (char *)topic;
temp->msg.payloadlen = payloadlen;
temp->msg.payload = (void *)payload;
temp->dest_ids = NULL;
temp->dest_id_count = 0;
if(!store_id){
temp->db_id = ++db->last_db_id;
}else{
temp->db_id = store_id;
}

temp->db = db;
LIST_INSERT_HEAD(&db->msg_store, temp, list);
db->msg_store_count++;

(*stored) = temp;
return MOSQ_ERR_SUCCESS;

no_memory:
_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory.");
if (new_source_id) {
_mosquitto_free(new_source_id);
}
if (new_topic) {
_mosquitto_free(new_topic);
}
if (new_payload) {
_mosquitto_free(new_payload);
}
if (temp) {
_mosquitto_free(temp);
}
return MOSQ_ERR_NOMEM;
}

2. Change implementation of mqtt3_db_message_store:

int mqtt3_db_message_store(struct mosquitto_db _db, const char *source, uint16_t source_mid, const char *topic, int qos, uint32_t payloadlen, const void *payload, int retain, struct mosquitto_msg_store *_stored, dbid_t store_id)
{
return __mqtt3_db_message_store(F_NEW_SOURCE_ID | F_NEW_TOPIC | F_NEW_PAYLOAD, db, source, source_mid, topic, qos, payloadlen, payload, retain, stored, store_id);
}

3. In mqtt3_handle_publish, call __mqtt3_db_message_store:

if(!stored){
dup = 0;
if(__mqtt3_db_message_store(F_NEW_SOURCE_ID | F_NEW_TOPIC, db, context->id, mid, topic, qos, payloadlen, payload, retain, &stored, 0)){
_mosquitto_free(topic);
if(payload) _mosquitto_free(payload);
return 1;
}
payload = NULL; <-- Add this line
}else{
dup = 1;
}

On 2015-06-28 17:28:05 -0400, Roger Light wrote:

Thanks for the report, I very much agree with the idea but I've implemented it a bit differently. It's in the develop branch.