Skip to content

Commit

Permalink
Hack to work around flush/refresh slowdown
Browse files Browse the repository at this point in the history 
NOT READY FOR COMMITTING!

This was a quick hack to remove the dirty metadata skip list's
lazy generation tweak introduced in 1.12.1.

This needs additional cleanup, especially in the test code, at the
bare minimum. A better solution would be to tweak the code so that
the lazy generation becomes "always" generation under certain
circumstances (SWMR, already called flush/refresh, etc.).
  • Loading branch information
@derobins
derobins committed Nov 11, 2023
1 parent e0d095e commit 5e31825
Show file tree
Hide file tree
Showing 14 changed files with 36 additions and 401 deletions.
98 changes: 0 additions & 98 deletions src/H5AC.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,28 +446,10 @@ H5AC_dest(H5F_t *f)
* Must not flush in the R/O case, as this will trigger the
* free space manager settle routines.
*
* Must also enable the skip list before the call to
* H5AC__flush_entries() and disable it afterwards, as the
* skip list will be disabled after the previous flush.
*
* Note that H5C_dest() does skip list setup and take down as well.
* Unfortunately, we can't do the setup and take down just once,
* as H5C_dest() is called directly in the test code.
*
* Fortunately, the cache should be clean or close to it at this
* point, so the overhead should be minimal.
*/
if (H5F_ACC_RDWR & H5F_INTENT(f)) {
/* enable and load the skip list */
if (H5C_set_slist_enabled(f->shared->cache, true, false) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "can't enable skip list");

if (H5AC__flush_entries(f) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "Can't flush");

/* disable the skip list -- should be empty */
if (H5C_set_slist_enabled(f->shared->cache, false, false) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "can't disable skip list");
} /* end if */
} /* end if */
#endif /* H5_HAVE_PARALLEL */
Expand Down Expand Up @@ -1095,86 +1077,6 @@ H5AC_prep_for_file_close(H5F_t *f)
FUNC_LEAVE_NOAPI(ret_value)
} /* H5AC_prep_for_file_close() */

/*-------------------------------------------------------------------------
*
* Function: H5AC_prep_for_file_flush
*
* Purpose: Handle any setup required prior to metadata cache flush.
*
* This function should be called just prior to the first
* call to H5AC_flush() during a file flush.
*
* Initially, this means setting up the skip list prior to the
* flush. We do this in a separate call because
* H5F__flush_phase2() make repeated calls to H5AC_flush().
* Handling this detail in separate calls allows us to avoid
* the overhead of setting up and taking down the skip list
* repeatedly.
*
* Return: Non-negative on success/Negative on failure
*
*-------------------------------------------------------------------------
*/
herr_t
H5AC_prep_for_file_flush(H5F_t *f)
{
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_NOAPI(FAIL)

/* Sanity checks */
assert(f);
assert(f->shared);
assert(f->shared->cache);

if (H5C_set_slist_enabled(f->shared->cache, true, false) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "can't enable skip list");

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* H5AC_prep_for_file_flush() */

/*-------------------------------------------------------------------------
*
* Function: H5AC_secure_from_file_flush
*
* Purpose: This function should be called just after the last
* call to H5AC_flush() during a file flush.
*
* The objective of the call is to allow the metadata cache
* to do any necessary necessary cleanup work after a cache
* flush.
*
* Initially, this means taking down the skip list after the
* flush. We do this in a separate call because
* H5F__flush_phase2() make repeated calls to H5AC_flush().
* Handling this detail in separate calls allows us to avoid
* the overhead of setting up and taking down the skip list
* repeatedly.
*
* Return: Non-negative on success/Negative on failure
*
*-------------------------------------------------------------------------
*/
herr_t
H5AC_secure_from_file_flush(H5F_t *f)
{
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_NOAPI(FAIL)

/* Sanity checks */
assert(f);
assert(f->shared);
assert(f->shared->cache);

if (H5C_set_slist_enabled(f->shared->cache, false, false) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "can't disable skip list");

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* H5AC_secure_from_file_flush() */

/*-------------------------------------------------------------------------
*
* Function: H5AC_create_flush_dependency()
Expand Down
2 changes: 0 additions & 2 deletions src/H5ACprivate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,8 +382,6 @@ H5_DLL herr_t H5AC_insert_entry(H5F_t *f, const H5AC_class_t *type, haddr_t addr
unsigned int flags);
H5_DLL herr_t H5AC_pin_protected_entry(void *thing);
H5_DLL herr_t H5AC_prep_for_file_close(H5F_t *f);
H5_DLL herr_t H5AC_prep_for_file_flush(H5F_t *f);
H5_DLL herr_t H5AC_secure_from_file_flush(H5F_t *f);
H5_DLL herr_t H5AC_create_flush_dependency(void *parent_thing, void *child_thing);
H5_DLL void *H5AC_protect(H5F_t *f, const H5AC_class_t *type, haddr_t addr, void *udata, unsigned flags);
H5_DLL herr_t H5AC_resize_entry(void *thing, size_t new_size);
Expand Down
151 changes: 1 addition & 150 deletions src/H5C.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ H5C_create(size_t max_cache_size, size_t min_clean_size, int max_type_id,
cache_ptr->num_objs_corked = 0;

/* slist field initializations */
cache_ptr->slist_enabled = false;
cache_ptr->slist_enabled = true;
cache_ptr->slist_changed = false;
cache_ptr->slist_len = 0;
cache_ptr->slist_size = (size_t)0;
Expand Down Expand Up @@ -492,10 +492,6 @@ H5C_dest(H5F_t *f)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "Can't display cache image stats");
#endif /* H5AC_DUMP_IMAGE_STATS_ON_CLOSE */

/* Enable the slist, as it is needed in the flush */
if (H5C_set_slist_enabled(f->shared->cache, true, false) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "set slist enabled failed");

/* Flush and invalidate all cache entries */
if (H5C__flush_invalidate_cache(f, H5C__NO_FLAGS_SET) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to flush cache");
Expand All @@ -505,13 +501,7 @@ H5C_dest(H5F_t *f)
if (H5C__generate_cache_image(f, cache_ptr) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTCREATE, FAIL, "Can't generate metadata cache image");

/* Question: Is it possible for cache_ptr->slist be non-null at this
* point? If no, shouldn't this if statement be an assert?
*/
if (cache_ptr->slist_ptr != NULL) {
assert(cache_ptr->slist_len == 0);
assert(cache_ptr->slist_size == 0);

H5SL_close(cache_ptr->slist_ptr);
cache_ptr->slist_ptr = NULL;
}
Expand All @@ -534,16 +524,6 @@ H5C_dest(H5F_t *f)
cache_ptr = H5FL_FREE(H5C_t, cache_ptr);

done:
if (ret_value < 0 && cache_ptr && cache_ptr->slist_ptr)
/* Arguably, it shouldn't be necessary to re-enable the slist after
* the call to H5C__flush_invalidate_cache(), as the metadata cache
* should be discarded. However, in the test code, we make multiple
* calls to H5C_dest(). Thus we re-enable the slist on failure if it
* and the cache still exist. JRM -- 5/15/20
*/
if (H5C_set_slist_enabled(f->shared->cache, false, false) < 0)
HDONE_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "disable slist on flush dest failure failed");

FUNC_LEAVE_NOAPI(ret_value)
} /* H5C_dest() */

Expand All @@ -566,18 +546,10 @@ H5C_evict(H5F_t *f)
/* Sanity check */
assert(f);

/* Enable the slist, as it is needed in the flush */
if (H5C_set_slist_enabled(f->shared->cache, true, false) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "set slist enabled failed");

/* Flush and invalidate all cache entries except the pinned entries */
if (H5C__flush_invalidate_cache(f, H5C__EVICT_ALLOW_LAST_PINS_FLAG) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to evict entries in the cache");

/* Disable the slist */
if (H5C_set_slist_enabled(f->shared->cache, false, true) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "set slist disabled failed");

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* H5C_evict() */
Expand Down Expand Up @@ -1021,127 +993,6 @@ H5C_set_evictions_enabled(H5C_t *cache_ptr, bool evictions_enabled)
FUNC_LEAVE_NOAPI(ret_value)
} /* H5C_set_evictions_enabled() */

/*-------------------------------------------------------------------------
* Function: H5C_set_slist_enabled()
*
* Purpose: Enable or disable the slist as directed.
*
* The slist (skip list) is an address ordered list of
* dirty entries in the metadata cache. However, this
* list is only needed during flush and close, where we
* use it to write entries in more or less increasing
* address order.
*
* This function sets up and enables further operations
* on the slist, or disable the slist. This in turn
* allows us to avoid the overhead of maintaining the
* slist when it is not needed.
*
*
* If the slist_enabled parameter is true, the function
*
* 1) Verifies that the slist is empty.
*
* 2) Scans the index list, and inserts all dirty entries
* into the slist.
*
* 3) Sets cache_ptr->slist_enabled = true.
*
* Note that the clear_slist parameter is ignored if
* the slist_enabed parameter is true.
*
*
* If the slist_enabled_parameter is false, the function
* shuts down the slist.
*
* Normally the slist will be empty at this point, however
* that need not be the case if H5C_flush_cache() has been
* called with the H5C__FLUSH_MARKED_ENTRIES_FLAG.
*
* Thus shutdown proceeds as follows:
*
* 1) Test to see if the slist is empty. If it is, proceed
* to step 3.
*
* 2) Test to see if the clear_slist parameter is true.
*
* If it is, remove all entries from the slist.
*
* If it isn't, throw an error.
*
* 3) set cache_ptr->slist_enabled = false.
*
* Return: SUCCEED on success, and FAIL on failure.
*
*-------------------------------------------------------------------------
*/
herr_t
H5C_set_slist_enabled(H5C_t *cache_ptr, bool slist_enabled, bool clear_slist)
{
H5C_cache_entry_t *entry_ptr;
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_NOAPI(FAIL)

if (cache_ptr == NULL)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "Bad cache_ptr on entry");

if (slist_enabled) {
if (cache_ptr->slist_enabled)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "slist already enabled?");
if ((cache_ptr->slist_len != 0) || (cache_ptr->slist_size != 0))
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "slist not empty?");

/* set cache_ptr->slist_enabled to true so that the slist
* maintenance macros will be enabled.
*/
cache_ptr->slist_enabled = true;

/* scan the index list and insert all dirty entries in the slist */
entry_ptr = cache_ptr->il_head;
while (entry_ptr != NULL) {
if (entry_ptr->is_dirty)
H5C__INSERT_ENTRY_IN_SLIST(cache_ptr, entry_ptr, FAIL);
entry_ptr = entry_ptr->il_next;
}

/* we don't maintain a dirty index len, so we can't do a cross
* check against it. Note that there is no point in cross checking
* against the dirty LRU size, as the dirty LRU may not be maintained,
* and in any case, there is no requirement that all dirty entries
* will reside on the dirty LRU.
*/
assert(cache_ptr->dirty_index_size == cache_ptr->slist_size);
}
else { /* take down the skip list */
if (!cache_ptr->slist_enabled)
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "slist already disabled?");

if ((cache_ptr->slist_len != 0) || (cache_ptr->slist_size != 0)) {
if (clear_slist) {
H5SL_node_t *node_ptr;

node_ptr = H5SL_first(cache_ptr->slist_ptr);
while (node_ptr != NULL) {
entry_ptr = (H5C_cache_entry_t *)H5SL_item(node_ptr);
H5C__REMOVE_ENTRY_FROM_SLIST(cache_ptr, entry_ptr, false, FAIL);
node_ptr = H5SL_first(cache_ptr->slist_ptr);
}
}
else
HGOTO_ERROR(H5E_CACHE, H5E_SYSTEM, FAIL, "slist not empty?");
}

cache_ptr->slist_enabled = false;

assert(0 == cache_ptr->slist_len);
assert(0 == cache_ptr->slist_size);
}

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* H5C_set_slist_enabled() */

/*-------------------------------------------------------------------------
* Function: H5C_unsettle_ring()
*
Expand Down
Loading

0 comments on commit 5e31825

Please sign in to comment.