Improve qp-trie compaction in write transactions

In general, it's better to do one thorough compaction when a batch of
work is complete, which is the way that `update` transactions work.
Conversely, `write` transactions are designed so that lots of little
transactions are not too inefficient, but they need explicit
compaction. This changes `dns_qp_compact()` so that it is easier to
compact any time that makes sense, if there isn't a better way to
schedule compaction. And `dns_qpmulti_commit()` only recycles garbage
when there is enough to make it worthwhile.

diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h
index ef676de7d435e06554074e1128c04b13813c221c..b089cc7db584e38e563c05ef7f8f5a0f98bf5479 100644 (file)
--- a/lib/dns/include/dns/qp.h
+++ b/lib/dns/include/dns/qp.h
@@ -230,6 +230,15 @@ typedef struct dns_qp_memusage {
        bool   fragmented;  /*%< trie needs compaction */
 } dns_qp_memusage_t;
 
+/*%
+ * Choice of mode for `dns_qp_compact()`
+ */
+typedef enum dns_qpgc {
+       DNS_QPGC_MAYBE,
+       DNS_QPGC_NOW,
+       DNS_QPGC_ALL,
+} dns_qpgc_t;
+
 /***********************************************************************
  *
  *  functions - create, destory, enquire
@@ -298,23 +307,28 @@ dns_qpmulti_destroy(dns_qpmulti_t **qpmp);
  */
 
 void
-dns_qp_compact(dns_qp_t *qp, bool all);
+dns_qp_compact(dns_qp_t *qp, dns_qpgc_t mode);
 /*%<
  * Defragment the qp-trie and release unused memory.
  *
  * When modifications make a trie too fragmented, it is automatically
  * compacted. However, automatic compaction is limited when a
  * multithreaded trie has lots of immutable memory from past
- * transactions, and lightweight write transactions do not do
+ * transactions, and lightweight write transactions do not compact on
+ * commit like heavyweight update transactions.
  *
  * This function can be used with a single-threaded qp-trie and during a
  * transaction on a multi-threaded trie.
  *
+ * \li If `mode == DNS_QPGC_MAYBE`, the trie is cleaned if it is fragmented
+ *
+ * \li If `mode == DNS_QPGC_NOW`, the trie is cleaned while avoiding
+ *     unnecessary work
+ *
+ * \li If `mode == DNS_QPGC_ALL`, the entire trie is compacted
+ *
  * Requires:
  * \li  `qp` is a pointer to a valid qp-trie
- * \li  `all` is true, to compact the whole trie
- * \li  `all` is false, to save time by not compacting
- *            chunk that are not fragmented
  */
 
 void

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index 1cdda0f8a5a30449cd2ceb3622fe1f1979062970..9c02c94619edbcb36fe2c1d335da24973ebf46aa 100644 (file)
--- a/lib/dns/qp.c
+++ b/lib/dns/qp.c
@@ -877,9 +877,14 @@ compact(dns_qp_t *qp) {
 }
 
 void
-dns_qp_compact(dns_qp_t *qp, bool all) {
+dns_qp_compact(dns_qp_t *qp, dns_qpgc_t mode) {
        REQUIRE(QP_VALID(qp));
-       qp->compact_all = all;
+       if (mode == DNS_QPGC_MAYBE && !QP_NEEDGC(qp)) {
+               return;
+       }
+       if (mode == DNS_QPGC_ALL) {
+               qp->compact_all = true;
+       }
        compact(qp);
        recycle(qp);
 }
@@ -907,7 +912,7 @@ dns_qp_compact(dns_qp_t *qp, bool all) {
 static inline bool
 squash_twigs(dns_qp_t *qp, qp_ref_t twigs, qp_weight_t size) {
        bool destroyed = free_twigs(qp, twigs, size);
-       if (destroyed && QP_MAX_GARBAGE(qp)) {
+       if (destroyed && QP_AUTOGC(qp)) {
                compact(qp);
                recycle(qp);
                /*
@@ -916,7 +921,7 @@ squash_twigs(dns_qp_t *qp, qp_ref_t twigs, qp_weight_t size) {
                 * time and space, but recovery should be cheaper than
                 * letting compact+recycle fail repeatedly.
                 */
-               if (QP_MAX_GARBAGE(qp)) {
+               if (QP_AUTOGC(qp)) {
                        isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE,
                                      DNS_LOGMODULE_QP, ISC_LOG_NOTICE,
                                      "qp %p uctx \"%s\" compact/recycle "
@@ -947,16 +952,9 @@ dns_qp_memusage(dns_qp_t *qp) {
                .free = qp->free_count,
                .node_size = sizeof(qp_node_t),
                .chunk_size = QP_CHUNK_SIZE,
+               .fragmented = QP_NEEDGC(qp),
        };
 
-       /*
-        * tell the caller about fragmentation of the whole trie
-        * without discounting immutable chunks
-        */
-       qp->hold_count = 0;
-       memusage.fragmented = QP_MAX_GARBAGE(qp);
-       qp->hold_count = memusage.hold;
-
        for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) {
                if (qp->base->ptr[chunk] != NULL) {
                        memusage.chunk_count += 1;
@@ -1037,7 +1035,7 @@ transaction_open(dns_qpmulti_t *multi, dns_qp_t **qptp) {
        }
 
        /*
-        * Ensure QP_MAX_GARBAGE() ignores free space in immutable chunks.
+        * Ensure QP_AUTOGC() ignores free space in immutable chunks.
         */
        qp->hold_count = qp->free_count;
 
@@ -1144,15 +1142,15 @@ dns_qpmulti_commit(dns_qpmulti_t *multi, dns_qp_t **qptp) {
                free_twigs(qp, multi->reader_ref, READER_SIZE);
        }
 
-       if (qp->transaction_mode == QP_WRITE) {
-               multi->reader_ref = alloc_twigs(qp, READER_SIZE);
-       } else {
+       if (qp->transaction_mode == QP_UPDATE) {
                /* minimize memory overhead */
                compact(qp);
                multi->reader_ref = alloc_twigs(qp, READER_SIZE);
                qp->base->ptr[qp->bump] = chunk_shrink_raw(
                        qp, qp->base->ptr[qp->bump],
                        qp->usage[qp->bump].used * sizeof(qp_node_t));
+       } else {
+               multi->reader_ref = alloc_twigs(qp, READER_SIZE);
        }
 
        /* anchor a new version of the trie */
@@ -1165,7 +1163,9 @@ dns_qpmulti_commit(dns_qpmulti_t *multi, dns_qp_t **qptp) {
        atomic_store_release(&multi->reader, reader); /* COMMIT */
 
        /* clean up what we can right now */
-       recycle(qp);
+       if (qp->transaction_mode == QP_UPDATE || QP_NEEDGC(qp)) {
+               recycle(qp);
+       }
 
        /* the reclamation phase must be sampled after the commit */
        isc_qsbr_phase_t phase = isc_qsbr_phase(multi->loopmgr);

diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h
index 5fc4cdf8018f1b3b56b8833b903258b0943daa78..6f9084e8316283650a05b9d6e3a2913f5107b487 100644 (file)
--- a/lib/dns/qp_p.h
+++ b/lib/dns/qp_p.h
@@ -171,9 +171,11 @@ STATIC_ASSERT(6 <= QP_CHUNK_LOG && QP_CHUNK_LOG <= 20,
  * commits. But they cannot be recovered immediately so they are also
  * counted as on hold, and discounted when we decide whether to compact.
  */
-#define QP_MAX_GARBAGE(qp)                                            \
-       (((qp)->free_count - (qp)->hold_count) > QP_CHUNK_SIZE * 4 && \
-        ((qp)->free_count - (qp)->hold_count) > (qp)->used_count / 2)
+#define QP_GC_HEURISTIC(qp, free) \
+       ((free) > QP_CHUNK_SIZE * 4 && (free) > (qp)->used_count / 2)
+
+#define QP_NEEDGC(qp) QP_GC_HEURISTIC(qp, (qp)->free_count)
+#define QP_AUTOGC(qp) QP_GC_HEURISTIC(qp, (qp)->free_count - (qp)->hold_count)
 
 /*
  * The chunk base and usage arrays are resized geometically and start off