Remove H5B debug checks (#4089)

The H5B (version 1 B-tree) package would add some computationally
expensive integrity checks when H5B_DEBUG was defined. Due to their
negative effects on performance, this option was rarely turned on,
making the H5B__assert() check function stale, if not dead, code.

This change:

* Builds H5B__assert() when NDEBUG is not defined (the function
  relies on assert()) so it gets compiled more often.
* Removes some printf debugging statements in the B-tree code
* Removes all H5B "extra debug" checks that are leftover from
  past debugging sessions. Maintainers can add H5B__assert()
  selectively to perform integrity checks when debugging.
* Removes the HDF5_ENABLE_DEBUG_H5B CMake option

H5B_DEBUG now has no effect

config/cmake/HDF5DeveloperBuild.cmake

@@ -139,16 +139,6 @@ if (HDF5_ENABLE_DEBUG_H5T_REF)
   list (APPEND HDF5_DEBUG_APIS H5T_REF_DEBUG)
 endif ()
 
-# HDF5 module debug definitions for debug code which may add
-# considerable amounts of overhead when enabled and is usually
-# only useful for specific circumstances rather than general
-# developer use.
-option (HDF5_ENABLE_DEBUG_H5B "Enable debugging of H5B module" OFF)
-mark_as_advanced (HDF5_ENABLE_DEBUG_H5B)
-if (HDF5_ENABLE_DEBUG_H5B)
-  list (APPEND HDF5_DEBUG_APIS H5B_DEBUG)
-endif ()
-
 option (HDF5_ENABLE_DEBUG_H5B2 "Enable debugging of H5B2 module" OFF)
 mark_as_advanced (HDF5_ENABLE_DEBUG_H5B2)
 if (HDF5_ENABLE_DEBUG_H5B2)

configure.ac

@@ -2572,7 +2572,7 @@ AC_SUBST([INTERNAL_DEBUG_OUTPUT])
 ## too specialized or have huge performance hits. These
 ## are not listed in the "all" packages list.
 ##
-## all_packages="AC,B,B2,D,F,FA,FL,FS,MM,O,S,T,Z"
+## all_packages="AC,B2,D,F,FA,FL,FS,MM,O,S,T,Z"
 all_packages="AC,B2,CX,D,F,MM,O,S,T,Z"
 
 case "X-$INTERNAL_DEBUG_OUTPUT" in

release_docs/RELEASE.txt

@@ -47,6 +47,13 @@ New Features
 
     Configuration:
     -------------
+    - The CMake HDF5_ENABLE_DEBUG_H5B option has been removed
+
+      This enabled some additional version-1 B-tree checks. These have been
+      removed so the option is no longer necessary.
+
+      This option was CMake-only and marked as advanced.
+
     - New option for building with static CRT in Windows
 
       The following option has been added:

src/H5B.c

@@ -12,9 +12,9 @@
 
 /*-------------------------------------------------------------------------
  *
- * Created:         H5B.c
+ * Created: H5B.c
  *
- * Purpose:		Implements balanced, sibling-linked, N-ary trees
+ * Purpose: Implements balanced, sibling-linked, N-ary trees
  *			capable of storing any type of data with unique key
  *			values.
  *
@@ -236,9 +236,6 @@ H5B_create(H5F_t *f, const H5B_class_t *type, void *udata, haddr_t *addr_p /*out
      */
     if (H5AC_insert_entry(f, H5AC_BT, *addr_p, bt, H5AC__NO_FLAGS_SET) < 0)
         HGOTO_ERROR(H5E_BTREE, H5E_CANTINIT, FAIL, "can't add B-tree root node to cache");
-#ifdef H5B_DEBUG
-    H5B__assert(f, *addr_p, shared->type, udata);
-#endif
 
 done:
     if (ret_value < 0) {
@@ -399,23 +396,6 @@ H5B__split(H5F_t *f, H5B_ins_ud_t *bt_ud, unsigned idx, void *udata, H5B_ins_ud_
     if (H5CX_get_btree_split_ratios(split_ratios) < 0)
         HGOTO_ERROR(H5E_BTREE, H5E_CANTGET, FAIL, "can't retrieve B-tree split ratios");
 
-#ifdef H5B_DEBUG
-    if (H5DEBUG(B)) {
-        const char *side;
-
-        if (!H5_addr_defined(bt_ud->bt->left) && !H5_addr_defined(bt_ud->bt->right))
-            side = "ONLY";
-        else if (!H5_addr_defined(bt_ud->bt->right))
-            side = "RIGHT";
-        else if (!H5_addr_defined(bt_ud->bt->left))
-            side = "LEFT";
-        else
-            side = "MIDDLE";
-        fprintf(H5DEBUG(B), "H5B__split: %3u {%5.3f,%5.3f,%5.3f} %6s", shared->two_k, split_ratios[0],
-                split_ratios[1], split_ratios[2], side);
-    }
-#endif
-
     /*
      * Decide how to split the children of the old node among the old node
      * and the new node.
@@ -437,10 +417,6 @@ H5B__split(H5F_t *f, H5B_ins_ud_t *bt_ud, unsigned idx, void *udata, H5B_ins_ud_
     else if (idx >= nleft && 0 == nleft)
         nleft++;
     nright = shared->two_k - nleft;
-#ifdef H5B_DEBUG
-    if (H5DEBUG(B))
-        fprintf(H5DEBUG(B), " split %3d/%-3d\n", nleft, nright);
-#endif
 
     /*
      * Create the new B-tree node.
@@ -649,11 +625,6 @@ H5B_insert(H5F_t *f, const H5B_class_t *type, haddr_t addr, void *udata)
         if (H5AC_unprotect(f, H5AC_BT, split_bt_ud.addr, split_bt_ud.bt, split_bt_ud.cache_flags) < 0)
             HDONE_ERROR(H5E_BTREE, H5E_CANTUNPROTECT, FAIL, "unable to unprotect new child");
 
-#ifdef H5B_DEBUG
-    if (ret_value >= 0)
-        H5B__assert(f, addr, type, udata);
-#endif
-
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5B_insert() */
 
@@ -944,14 +915,9 @@ H5B__insert_helper(H5F_t *f, H5B_ins_ud_t *bt_ud, const H5B_class_t *type, uint8
 #endif /* H5_STRICT_FORMAT_CHECKS */
     }
     else if (cmp) {
-        /*
-         * We couldn't figure out which branch to follow out of this node. THIS
-         * IS A MAJOR PROBLEM THAT NEEDS TO BE FIXED --rpm.
-         */
-        assert("INTERNAL HDF5 ERROR (contact rpm)" && 0);
-#ifdef NDEBUG
-        HDabort();
-#endif /* NDEBUG */
+        /* We couldn't figure out which branch to follow out of this node */
+        HGOTO_ERROR(H5E_BTREE, H5E_CANTINSERT, H5B_INS_ERROR,
+                    "internal error: could not determine which branch to follow out of this node");
     }
     else if (bt->level > 0) {
         /*
@@ -1054,15 +1020,7 @@ H5B__insert_helper(H5F_t *f, H5B_ins_ud_t *bt_ud, const H5B_class_t *type, uint8
     if (split_bt_ud->bt) {
         H5MM_memcpy(md_key, H5B_NKEY(split_bt_ud->bt, shared, 0), type->sizeof_nkey);
         ret_value = H5B_INS_RIGHT;
-#ifdef H5B_DEBUG
-        /*
-         * The max key in the original left node must be equal to the min key
-         * in the new node.
-         */
-        cmp = (type->cmp2)(H5B_NKEY(bt, shared, bt->nchildren), udata, H5B_NKEY(split_bt_ud->bt, shared, 0));
-        assert(0 == cmp);
-#endif
-    } /* end if */
+    }
     else
         ret_value = H5B_INS_NOOP;
 
@@ -1544,9 +1502,6 @@ H5B_remove(H5F_t *f, const H5B_class_t *type, haddr_t addr, void *udata)
         H5B__remove_helper(f, addr, type, 0, lt_key, &lt_key_changed, udata, rt_key, &rt_key_changed))
         HGOTO_ERROR(H5E_BTREE, H5E_CANTINIT, FAIL, "unable to remove entry from B-tree");
 
-#ifdef H5B_DEBUG
-    H5B__assert(f, addr, type, udata);
-#endif
 done:
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5B_remove() */

src/H5Bdbg.c

@@ -14,7 +14,7 @@
  *
  * Created:         H5Bdbg.c
  *
- * Purpose:         Debugging routines for B-link tree package.
+ * Purpose:         Debugging routines for B-link tree package
  *
  *-------------------------------------------------------------------------
  */
@@ -36,10 +36,9 @@
 /*-------------------------------------------------------------------------
  * Function:    H5B_debug
  *
- * Purpose:     Prints debugging info about a B-tree.
+ * Purpose:     Prints debugging info about a B-tree
  *
  * Return:      Non-negative on success/Negative on failure
- *
  *-------------------------------------------------------------------------
  */
 herr_t
@@ -132,15 +131,15 @@ H5B_debug(H5F_t *f, haddr_t addr, FILE *stream, int indent, int fwidth, const H5
 /*-------------------------------------------------------------------------
  * Function:    H5B__assert
  *
- * Purpose:     Verifies that the tree is structured correctly.
- *
- * Return:      Success:    SUCCEED
+ * Purpose:     Verifies that the tree is structured correctly
  *
- *              Failure:    aborts if something is wrong.
+ *              Relies on assert(), so only built when NDEBUG is not set
  *
+ * Return:      Success:    SUCCEED
+ *              Failure:    assert()
  *-------------------------------------------------------------------------
  */
-#ifdef H5B_DEBUG
+#ifndef NDEBUG
 herr_t
 H5B__assert(H5F_t *f, haddr_t addr, const H5B_class_t *type, void *udata)
 {
@@ -149,7 +148,6 @@ H5B__assert(H5F_t *f, haddr_t addr, const H5B_class_t *type, void *udata)
     H5B_shared_t  *shared;      /* Pointer to shared B-tree info */
     H5B_cache_ud_t cache_udata; /* User-data for metadata cache callback */
     int            ncell, cmp;
-    static int     ncalls = 0;
     herr_t         status;
     herr_t         ret_value = SUCCEED; /* Return value */
 
@@ -162,11 +160,6 @@ H5B__assert(H5F_t *f, haddr_t addr, const H5B_class_t *type, void *udata)
 
     FUNC_ENTER_PACKAGE
 
-    if (0 == ncalls++) {
-        if (H5DEBUG(B))
-            fprintf(H5DEBUG(B), "H5B: debugging B-trees (expensive)\n");
-    } /* end if */
-
     /* Get shared info for B-tree */
     if (NULL == (rc_shared = (type->get_shared)(f, udata)))
         HGOTO_ERROR(H5E_BTREE, H5E_CANTGET, FAIL, "can't retrieve B-tree's shared ref. count object");
@@ -257,4 +250,4 @@ H5B__assert(H5F_t *f, haddr_t addr, const H5B_class_t *type, void *udata)
 done:
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5B__assert() */
-#endif /* H5B_DEBUG */
+#endif /* !NDEBUG */

src/H5Bpkg.h

@@ -78,7 +78,7 @@ H5FL_EXTERN(H5B_t);
 /* Package Private Prototypes */
 /******************************/
 H5_DLL herr_t H5B__node_dest(H5B_t *bt);
-#ifdef H5B_DEBUG
+#ifndef NDEBUG
 herr_t H5B__assert(H5F_t *f, haddr_t addr, const H5B_class_t *type, void *udata);
 #endif
 

src/H5Bprivate.h

@@ -31,16 +31,6 @@
 /* Library Private Macros */
 /**************************/
 
-/*
- * Feature: Define this constant if you want to check B-tree consistency
- *	    after each B-tree operation.  Note that this slows down the
- *	    library considerably! Debugging the B-tree depends on assert()
- *	    being enabled.
- */
-#ifdef NDEBUG
-#undef H5B_DEBUG
-#endif
-
 /****************************/
 /* Library Private Typedefs */
 /****************************/