From dd9aceb71313b6370710ce0896f5662346d8828d Mon Sep 17 00:00:00 2001 From: Erik Boasson Date: Tue, 19 Feb 2019 10:53:09 +0100 Subject: [PATCH] small performance improvement in RHC The introduction of properly functioning query conditions adds some overhead, this commit removes some of that cost by avoiding some calls to update_conditions when there are no query conditions. It also removes the has_changed field from the instance, instead using a local boolean to track whether DATA_AVAILABLE should be raised or not. Signed-off-by: Erik Boasson --- src/core/ddsc/src/dds_rhc.c | 76 ++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/src/core/ddsc/src/dds_rhc.c b/src/core/ddsc/src/dds_rhc.c index 30dc4f3..7f3029b 100644 --- a/src/core/ddsc/src/dds_rhc.c +++ b/src/core/ddsc/src/dds_rhc.c @@ -250,7 +250,6 @@ struct rhc_instance { unsigned isnew : 1; /* NEW or NOT_NEW view state */ unsigned a_sample_free : 1; /* whether or not a_sample is in use */ unsigned isdisposed : 1; /* DISPOSED or NOT_DISPOSED (if not disposed, wrcount determines ALIVE/NOT_ALIVE_NO_WRITERS) */ - unsigned has_changed : 1; /* To track changes in an instance - if number of samples are added or data is overwritten */ unsigned wr_iid_islive : 1; /* whether wr_iid is of a live writer */ unsigned inv_exists : 1; /* whether or not state change occurred since last sample (i.e., must return invalid sample) */ unsigned inv_isread : 1; /* whether or not that state change has been read before */ @@ -313,7 +312,6 @@ struct trigger_info_cmn { unsigned qminst; bool has_read; bool has_not_read; - bool has_changed; }; struct trigger_info_pre { @@ -552,7 +550,7 @@ static void inst_clear_invsample_if_exists (struct rhc *rhc, struct rhc_instance inst_clear_invsample (rhc, inst, trig_qc); } -static void inst_set_invsample (struct rhc *rhc, struct rhc_instance *inst, struct trigger_info_qcond *trig_qc) +static void inst_set_invsample (struct rhc *rhc, struct rhc_instance *inst, struct trigger_info_qcond *trig_qc, bool *nda) { if (!inst->inv_exists || inst->inv_isread) { @@ -563,6 +561,7 @@ static void inst_set_invsample (struct rhc *rhc, struct rhc_instance *inst, stru inst->inv_exists = 1; inst->inv_isread = 0; rhc->n_invsamples++; + *nda = true; } } @@ -651,7 +650,6 @@ static void init_trigger_info_cmn_nonmatch (struct trigger_info_cmn *info) info->qminst = ~0u; info->has_read = false; info->has_not_read = false; - info->has_changed = false; } static void get_trigger_info_cmn (struct trigger_info_cmn *info, struct rhc_instance *inst) @@ -659,12 +657,10 @@ static void get_trigger_info_cmn (struct trigger_info_cmn *info, struct rhc_inst info->qminst = qmask_of_inst (inst); info->has_read = inst_has_read (inst); info->has_not_read = inst_has_unread (inst); - info->has_changed = inst->has_changed; } static void get_trigger_info_pre (struct trigger_info_pre *info, struct rhc_instance *inst) { - inst->has_changed = 0; get_trigger_info_cmn (&info->c, inst); } @@ -680,15 +676,19 @@ static void init_trigger_info_qcond (struct trigger_info_qcond *qc) qc->inc_conds_sample = 0; } -static bool trigger_info_differs (const struct trigger_info_pre *pre, const struct trigger_info_post *post, const struct trigger_info_qcond *trig_qc) +static bool trigger_info_differs (const struct rhc *rhc, const struct trigger_info_pre *pre, const struct trigger_info_post *post, const struct trigger_info_qcond *trig_qc) { - return (pre->c.qminst != post->c.qminst || - pre->c.has_read != post->c.has_read || - pre->c.has_not_read != post->c.has_not_read || - pre->c.has_changed != post->c.has_changed || - trig_qc->dec_conds_invsample != trig_qc->inc_conds_invsample || - trig_qc->dec_conds_sample != trig_qc->inc_conds_sample || - trig_qc->dec_invsample_read != trig_qc->inc_invsample_read); + if (pre->c.qminst != post->c.qminst || + pre->c.has_read != post->c.has_read || + pre->c.has_not_read != post->c.has_not_read) + return true; + else if (rhc->nqconds == 0) + return false; + else + return (trig_qc->dec_conds_invsample != trig_qc->inc_conds_invsample || + trig_qc->dec_conds_sample != trig_qc->inc_conds_sample || + trig_qc->dec_invsample_read != trig_qc->inc_invsample_read || + trig_qc->dec_sample_read != trig_qc->inc_sample_read); } static bool add_sample (struct rhc *rhc, struct rhc_instance *inst, const struct proxy_writer_info *pwr_info, const struct ddsi_serdata *sample, status_cb_data_t *cb_data, struct trigger_info_qcond *trig_qc) @@ -722,9 +722,6 @@ static bool add_sample (struct rhc *rhc, struct rhc_instance *inst, const struct inst->nvread--; rhc->n_vread--; } - - /* set a flag to indicate instance has changed to notify data_available since the sample is overwritten */ - inst->has_changed = 1; } else { @@ -1042,7 +1039,7 @@ static int rhc_unregister_isreg_w_sideeffects (struct rhc *rhc, const struct rhc } } -static int rhc_unregister_updateinst (struct rhc *rhc, struct rhc_instance *inst, const struct proxy_writer_info * __restrict pwr_info, nn_wctime_t tstamp, struct trigger_info_qcond *trig_qc) +static int rhc_unregister_updateinst (struct rhc *rhc, struct rhc_instance *inst, const struct proxy_writer_info * __restrict pwr_info, nn_wctime_t tstamp, struct trigger_info_qcond *trig_qc, bool *nda) { assert (inst->wrcount > 0); @@ -1074,7 +1071,7 @@ static int rhc_unregister_updateinst (struct rhc *rhc, struct rhc_instance *inst care.) */ if (inst->latest == NULL || inst->latest->isread) { - inst_set_invsample (rhc, inst, trig_qc); + inst_set_invsample (rhc, inst, trig_qc, nda); update_inst (inst, pwr_info, false, tstamp); } if (!inst->isdisposed) @@ -1096,7 +1093,7 @@ static int rhc_unregister_updateinst (struct rhc *rhc, struct rhc_instance *inst /* Add invalid samples for transition to no-writers */ TRACE (",#0,empty,nowriters"); assert (inst_is_empty (inst)); - inst_set_invsample (rhc, inst, trig_qc); + inst_set_invsample (rhc, inst, trig_qc, nda); update_inst (inst, pwr_info, false, tstamp); account_for_empty_to_nonempty_transition (rhc, inst); inst->wr_iid_islive = 0; @@ -1105,8 +1102,10 @@ static int rhc_unregister_updateinst (struct rhc *rhc, struct rhc_instance *inst } } -static void dds_rhc_unregister (struct rhc *rhc, struct rhc_instance *inst, const struct proxy_writer_info * __restrict pwr_info, nn_wctime_t tstamp, struct trigger_info_post *post, struct trigger_info_qcond *trig_qc) +static bool dds_rhc_unregister (struct rhc *rhc, struct rhc_instance *inst, const struct proxy_writer_info * __restrict pwr_info, nn_wctime_t tstamp, struct trigger_info_post *post, struct trigger_info_qcond *trig_qc) { + bool notify_data_available = false; + /* 'post' always gets set; instance may have been freed upon return. */ TRACE (" unregister:"); if (!rhc_unregister_isreg_w_sideeffects (rhc, inst, pwr_info->iid)) @@ -1114,7 +1113,7 @@ static void dds_rhc_unregister (struct rhc *rhc, struct rhc_instance *inst, cons /* other registrations remain */ get_trigger_info_cmn (&post->c, inst); } - else if (rhc_unregister_updateinst (rhc, inst, pwr_info, tstamp, trig_qc)) + else if (rhc_unregister_updateinst (rhc, inst, pwr_info, tstamp, trig_qc, ¬ify_data_available)) { /* instance dropped */ init_trigger_info_cmn_nonmatch (&post->c); @@ -1124,6 +1123,7 @@ static void dds_rhc_unregister (struct rhc *rhc, struct rhc_instance *inst, cons /* no writers remain, but instance not empty */ get_trigger_info_cmn (&post->c, inst); } + return notify_data_available; } static struct rhc_instance *alloc_new_instance (const struct rhc *rhc, const struct proxy_writer_info *pwr_info, struct ddsi_serdata *serdata, struct ddsi_tkmap_instance *tk) @@ -1207,7 +1207,8 @@ static rhc_store_result_t rhc_store_new_instance (struct rhc_instance **out_inst else { if (inst->isdisposed) { - inst_set_invsample (rhc, inst, trig_qc); + bool nda_dummy = false; + inst_set_invsample (rhc, inst, trig_qc, &nda_dummy); } } @@ -1242,6 +1243,7 @@ bool dds_rhc_store (struct rhc * __restrict rhc, const struct proxy_writer_info rhc_store_result_t stored; status_cb_data_t cb_data; /* Callback data for reader status callback */ bool delivered = true; + bool notify_data_available = false; TRACE ("rhc_store(%"PRIx64",%"PRIx64" si %x has_data %d:", tk->m_iid, wr_iid, statusinfo, has_data); if (!has_data && statusinfo == 0) @@ -1282,6 +1284,7 @@ bool dds_rhc_store (struct rhc * __restrict rhc, const struct proxy_writer_info goto error_or_nochange; } init_trigger_info_cmn_nonmatch (&pre.c); + notify_data_available = true; } } else if (!inst_accepts_sample (rhc, inst, pwr_info, sample, has_data)) @@ -1300,7 +1303,8 @@ bool dds_rhc_store (struct rhc * __restrict rhc, const struct proxy_writer_info } if (statusinfo & NN_STATUSINFO_UNREGISTER) { - dds_rhc_unregister (rhc, inst, pwr_info, sample->timestamp, &post, &trig_qc); + if (dds_rhc_unregister (rhc, inst, pwr_info, sample->timestamp, &post, &trig_qc)) + notify_data_available = true; } else { @@ -1389,11 +1393,12 @@ bool dds_rhc_store (struct rhc * __restrict rhc, const struct proxy_writer_info inst->disposed_gen--; goto error_or_nochange; } + notify_data_available = true; } /* If instance became disposed, add an invalid sample if there are no samples left */ if (inst_became_disposed && inst->latest == NULL) - inst_set_invsample (rhc, inst, &trig_qc); + inst_set_invsample (rhc, inst, &trig_qc, ¬ify_data_available); update_inst (inst, pwr_info, true, sample->timestamp); @@ -1449,18 +1454,7 @@ bool dds_rhc_store (struct rhc * __restrict rhc, const struct proxy_writer_info TRACE (")\n"); - const bool update_read_conditions = trigger_info_differs (&pre, &post, &trig_qc); - bool notify_data_available; - /* do not send data available notification when an instance is dropped */ - if ((post.c.qminst == ~0u) && (post.c.has_read == 0) && (post.c.has_not_read == 0) && (post.c.has_changed == false)) - notify_data_available = false; - else /* FIXME: now that trigger_info_differs incorporates details on samples added/removed, this might well be wrong */ - notify_data_available = update_read_conditions; - - if (update_read_conditions) - trigger_waitsets = update_conditions_locked (rhc, true, &pre, &post, &trig_qc, inst); - else - trigger_waitsets = false; + trigger_waitsets = trigger_info_differs (rhc, &pre, &post, &trig_qc) && update_conditions_locked (rhc, true, &pre, &post, &trig_qc, inst); assert (rhc_check_counts_locked (rhc, true, true)); @@ -1557,7 +1551,7 @@ void dds_rhc_unregister_wr (struct rhc * __restrict rhc, const struct proxy_writ else { const bool was_empty = inst_is_empty (inst); - inst_set_invsample (rhc, inst, &trig_qc); + inst_set_invsample (rhc, inst, &trig_qc, ¬ify_data_available); if (was_empty) account_for_empty_to_nonempty_transition (rhc, inst); else @@ -1570,7 +1564,7 @@ void dds_rhc_unregister_wr (struct rhc * __restrict rhc, const struct proxy_writ TRACE ("\n"); notify_data_available = true; - if (trigger_info_differs (&pre, &post, &trig_qc) && update_conditions_locked (rhc, true, &pre, &post, &trig_qc, inst)) + if (trigger_info_differs (rhc, &pre, &post, &trig_qc) && update_conditions_locked (rhc, true, &pre, &post, &trig_qc, inst)) trigger_waitsets = true; assert (rhc_check_counts_locked (rhc, true, false)); } @@ -1589,7 +1583,9 @@ void dds_rhc_unregister_wr (struct rhc * __restrict rhc, const struct proxy_writ } if (trigger_waitsets) - dds_entity_status_signal (&rhc->reader->m_entity); + { + dds_entity_status_signal(&rhc->reader->m_entity); + } } }