Cache result of FSStore._fsspec_installed()

[ph03]

FSStore._fsspec_installed() is run in a lot of hierarchy operations, and has a significant runtime overhead for each invocation. Given the result will not change between invocations, cache the very first invocation for subsequent runs.

Example profiling of effects without (main) behaviour (6sec total runtime)

and with caching the invocation (2sec total runtime)

Most of the runtime is spent in the repeated lookup of FSStore._fsspec_installed()

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[d-v-b]

i was just noticing sluggish fsstore performance today. this change looks good to me!

[codecov]

Codecov Report

Merging #1581 (b0fed12) into main (d40bf12) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1581   +/-   ##
=======================================
  Coverage   99.99%   99.99%           
=======================================
  Files          37       37           
  Lines       14741    14743    +2     
=======================================
+ Hits        14740    14742    +2     
  Misses          1        1           

Files	Coverage Δ
zarr/storage.py	100.00% <100.00%> (ø)

[jhamman]

This looks good to me. Would be nice to get @martindurant to take a look here.

[martindurant]

Agree. I suppose checking "fsspec" in sys.modules would be much faster, after the first use, but this is fine.

Not sure why we didn't do something more typical like

        if not self._fsspec_installed():  # pragma: no cover
            raise ImportError("`fsspec` is required to use zarr's FSStore")

->

        try:
            import fsspec
        except ImportError:

[ph03]

Should I still adapt this?

[martindurant]

I am happy to go with @d-v-b and @jhamman on this one - this fix is fine, and we can always simplify another time.

[ph03]

I am happy to go with @d-v-b and @jhamman on this one - this fix is fine, and we can always simplify another time.

Awesome - perhaps you can let me know if there are additional points missing for merging?

[jhamman]

I think the main missing thing here is a changelog entry. Then we should be good to merge.

[ph03]

I think the main missing thing here is a changelog entry. Then we should be good to merge.

Cool thanks, I wasn't aware, thanks for pointing out - added a changelog entry now :)

[ph03]

@martindurant are you ok with the PR / can you let me know which remaining parts are missing (would like to start pointing dependent code to a main repo commit / not a personal one ^^)?