From 3d09a7d5f6dd0f602a8f343809384ed00cd13f5c Mon Sep 17 00:00:00 2001 From: Dana Robinson <43805+derobins@users.noreply.github.com> Date: Fri, 8 Mar 2024 23:17:13 -0800 Subject: 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 | 10 ------ configure.ac | 2 +- release_docs/RELEASE.txt | 7 +++++ src/H5B.c | 57 ++++------------------------------- src/H5Bdbg.c | 23 +++++--------- src/H5Bpkg.h | 2 +- src/H5Bprivate.h | 10 ------ 7 files changed, 23 insertions(+), 88 deletions(-) diff --git a/config/cmake/HDF5DeveloperBuild.cmake b/config/cmake/HDF5DeveloperBuild.cmake index 40efb0e..f8ccc2f 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 30962b1..df49390 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,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 diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index e80f5ba..279e9cc 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 9d89218..6032943 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 @@ done: 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, <_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 73eccec..9b12da1 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 @@ done: /*------------------------------------------------------------------------- * 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 f50bd48..d181867 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 a1d84ef..f93fa9c 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 */ /****************************/ -- cgit v0.12