From 74c74172bdb89c2b428961788b397d8a889b20a2 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Thu, 7 Mar 2024 23:54:15 -0800 Subject: [PATCH] Remove H5B debug checks 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 | 10 ----- configure.ac | 2 +- release_docs/RELEASE.txt | 7 ++++ src/H5B.c | 56 +++------------------------ src/H5Bdbg.c | 23 ++++------- src/H5Bpkg.h | 2 +- src/H5Bprivate.h | 10 ----- 7 files changed, 22 insertions(+), 88 deletions(-) diff --git a/config/cmake/HDF5DeveloperBuild.cmake b/config/cmake/HDF5DeveloperBuild.cmake index 40efb0e0711..f8ccc2f7bef 100644 --- a/config/cmake/HDF5DeveloperBuild.cmake +++ b/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) diff --git a/configure.ac b/configure.ac index 26c7681d917..fac67d067ba 100644 --- a/configure.ac +++ b/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,I,MM,O,S,T,Z" +## all_packages="AC,B2,D,F,FA,FL,FS,I,MM,O,S,T,Z" all_packages="AC,B2,CX,D,F,I,MM,O,S,T,Z" case "X-$INTERNAL_DEBUG_OUTPUT" in diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index e80f5ba5edd..279e9ccd690 100644 --- a/release_docs/RELEASE.txt +++ b/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: diff --git a/src/H5B.c b/src/H5B.c index 9d89218d62f..a7133aa290f 100644 --- a/src/H5B.c +++ b/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,8 @@ 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 */ + assert("INTERNAL HDF5 ERROR" && 0); } else if (bt->level > 0) { /* @@ -1054,15 +1019,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 +1501,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, <_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() */ diff --git a/src/H5Bdbg.c b/src/H5Bdbg.c index 73eccecd0a8..9b12da170ea 100644 --- a/src/H5Bdbg.c +++ b/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 */ diff --git a/src/H5Bpkg.h b/src/H5Bpkg.h index f50bd48ac2e..d18186774d4 100644 --- a/src/H5Bpkg.h +++ b/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 diff --git a/src/H5Bprivate.h b/src/H5Bprivate.h index a1d84efd1e7..f93fa9c5d82 100644 --- a/src/H5Bprivate.h +++ b/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 */ /****************************/