(#509) Fix off-by-1 issues re. MAX_THREADS in clockcache.c

This commit fixes off-by-1 issues when checking MAX_THREADS value in assertions in clockcache.c . These problems are easily reproduced with driver_test splinter_test with max # of threads. A new test execution with --num-insert-threads 63 is added to nightly runs, in test.sh .
vmware · Jan 6, 2023 · 6ff1a81 · 6ff1a81
1 parent 615050a
commit 6ff1a81
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 5 deletions.
diff --git a/src/clockcache.c b/src/clockcache.c
@@ -816,13 +816,25 @@ clockcache_inc_ref(clockcache *cc, uint32 entry_number, threadid counter_no)
 static inline void
 clockcache_dec_ref(clockcache *cc, uint32 entry_number, threadid counter_no)
 {
+   debug_only threadid input_counter_no = counter_no;
+
    counter_no %= CC_RC_WIDTH;
    uint64 rc_number = clockcache_get_ref_internal(cc, entry_number);
-   debug_assert(rc_number < cc->cfg->page_capacity);
+   debug_assert((rc_number < cc->cfg->page_capacity),
+                "Entry number, %lu, is out of allocator "
+                "page capacity range, %u.\n",
+                rc_number,
+                cc->cfg->page_capacity);
 
    debug_only uint16 refcount = __sync_fetch_and_sub(
       &cc->refcount[counter_no * cc->cfg->page_capacity + rc_number], 1);
-   debug_assert(refcount != 0);
+   debug_assert((refcount != 0),
+                "Invalid refcount, %u, after decrement."
+                " input counter_no=%lu, rc_number=%lu, counter_no=%lu\n",
+                refcount,
+                input_counter_no,
+                rc_number,
+                counter_no);
 }
 
 static inline uint8
@@ -868,7 +880,7 @@ clockcache_assert_no_refs(clockcache *cc)
 {
    threadid        i;
    volatile uint32 j;
-   for (i = 0; i < (MAX_THREADS - 1); i++) {
+   for (i = 0; i < MAX_THREADS; i++) {
       for (j = 0; j < cc->cfg->page_capacity; j++) {
          if (clockcache_get_ref(cc, j, i) != 0) {
             clockcache_get_ref(cc, j, i);
@@ -1304,7 +1316,7 @@ clockcache_batch_start_writeback(clockcache *cc, uint64 batch, bool is_urgent)
 
    clockcache_entry *entry, *next_entry;
 
-   debug_assert(tid < MAX_THREADS - 1);
+   debug_assert((tid < MAX_THREADS), "Invalid tid=%lu\n", tid);
    debug_assert(cc != NULL);
    debug_assert(batch < cc->cfg->page_capacity / CC_ENTRIES_PER_BATCH);
 
@@ -1594,7 +1606,7 @@ clockcache_get_free_page(clockcache *cc,
    clockcache_entry *entry;
    timestamp         wait_start;
 
-   debug_assert(tid < MAX_THREADS);
+   debug_assert((tid < MAX_THREADS), "Invalid tid=%lu\n", tid);
    if (cc->per_thread[tid].free_hand == CC_UNMAPPED_ENTRY) {
       clockcache_move_hand(cc, FALSE);
    }

diff --git a/test.sh b/test.sh
@@ -270,6 +270,21 @@ function nightly_sync_perf_tests() {
                                                 --db-capacity-gib 60 \
                                                 --db-location ${dbname}
     rm ${dbname}
+
+    # Exercise a case with max # of insert-threads which tripped an assertion
+    # This isn't really a 'perf' test but a regression / stability test exec
+    nins_t=63
+    nrange_lookup_t=0
+    test_descr="${nins_t} insert threads"
+    dbname="splinter_test.max_threads.db"
+
+    run_with_timing "Performance with max-threads ${test_descr}" \
+            "$BINDIR"/driver_test splinter_test --perf \
+                                                --num-insert-threads ${nins_t} \
+                                                --num-range-lookup-threads ${nrange_lookup_t} \
+                                                --tree-size-gib 1 \
+                                                --db-location ${dbname}
+    rm ${dbname}
 }
 
 # #############################################################################