Skip to content

Cleanup the handling of cache parameters.#2734

Merged
WardF merged 2 commits intoUnidata:mainfrom
DennisHeimbigner:cachesizes.dmh
Aug 11, 2023
Merged

Cleanup the handling of cache parameters.#2734
WardF merged 2 commits intoUnidata:mainfrom
DennisHeimbigner:cachesizes.dmh

Conversation

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator

@DennisHeimbigner DennisHeimbigner commented Aug 10, 2023

re: Issue #2733

When addressing the above issue, I noticed that there was a disconnect in NCZarr between nc_set_chunk_cache and nc_set_var_chunk cache. Specifically, setting nc_set_chunk_cache had no impact on the per-variable cache parameters when nc_set_var_chunk_cache was not used.

So, modified the NCZarr code so that the per-variable cache parameters are set in this order (#1 is first choice):

  1. The values set by nc_set_var_chunk_cache
  2. The values set by nc_set_chunk_cache
  3. The defaults set by configure.ac

re: Unidata#2733

When addressing the above issue, I noticed that there was a disconnect
in NCZarr between nc_set_chunk_cache and nc_set_var_chunk cache.
Specifically, setting nc_set_chunk_cache had no impact on the per-variable cache parameters when nc_set_var_chunk_cache was not used.

So, modified the NCZarr code so that the per-variable cache parameters are set in this order (#1 is first choice):
1. The values set by nc_set_var_chunk_cache
2. The values set by nc_set_chunk_cache
3. The defaults set by configure.ac
WardF
WardF previously approved these changes Aug 10, 2023
@WardF WardF merged commit f72e3cb into Unidata:main Aug 11, 2023
DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this pull request Aug 17, 2023
re: PR Unidata#2734
re: Issue Unidata#2733

As a result of an investigation by https://github.com/uweschulzweida,
I discovered a significant bug in the NCZarr cache management.
This PR extends the above PR to fix that bug.

## Change Overview
* Insert extra checks for cache overflow.
* Added test cases contingent on the --enable-large-file-tests option.
* The Columbia server is down, so it has been temporarily disabled.
@DennisHeimbigner DennisHeimbigner deleted the cachesizes.dmh branch September 27, 2023 18:52
@edwardhartnett
Copy link
Copy Markdown
Contributor

OK, I think I see the problem here.

IIRC, HDF5 has two different chunk cache settings, one for the file chuck cache (nc_set_chunk_cache) and the variable chunk cache. They are different, but seem to be conflated in this PR.

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator Author

My understanding was that the file-level chunk cache is only a set of size parameters and there is no
actual file level cache object. It is only variables that have actual cache objects.
Is this incorrect for HDF5? If so, then when is the file chunk cache used?

@WardF
Copy link
Copy Markdown
Member

WardF commented Jan 14, 2025

I'm untangling this today as well to see where MAX_DEFAULT_CACHE_SIZE was being used prior to this. I may re-add it for the full release with a 'deprecated' tag, unless we can determine it serves a separate purpose. I'm just focusing on untying this logic knot around these similarly named variables. Regarding #3067, and the observed performance degradation, I am inclined to make the changes necessary to the default values to restore previous performance, while also making it more clear what the options represent, as need be.

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator Author

I agree. The first priority is getting performance back to the way it was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants