Uncached os.listdir() in _get_family() causes much slower resolves on Windows

[mandeep]

Hi all,

I was seeing extremely slow resolve times on Windows, even with memcached showing a 99% hit rate. I caught that os.listdir() was being called for every package check. I changed this to use the decorated _get_familydirs() and noticed that resolve times were now instant. I'm new to Rez so I don't know if this is the correct way to handle this, but it seems to be working now as intended.

Thanks!
Mandeep

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mandeep / name: mandeep (f0006eb)

[JeanChristopheMorinPerso]

Hi @mandeep , thanks for opening this PR. Do you have more details on the performance slow down you were getting and what you get now with these changes? Extreme and massive are two big adjectives. Not saying that it wasn't the case, but I'd like to see some numbers to back this up and help us a bit.

When talking about performance, it's important to provide some numbers and more context to get an idea of the scale of the problem and changes. Things like what kind of performance you were getting before, and what you get now, how many directories are in your repos, how many repos, how many packages, type of filesystem, etc.

It will also help us determine if the problem is this or if it's related to something (like the cache for example).

Thanks!

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.33%. Comparing base (73a094d) to head (f0006eb).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/rezplugins/package_repository/filesystem.py	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2195      +/-   ##
==========================================
+ Coverage   61.30%   61.33%   +0.03%     
==========================================
  Files         164      164              
  Lines       20572    20575       +3     
  Branches     3575     3578       +3     
==========================================
+ Hits        12611    12620       +9     
+ Misses       7089     7085       -4     
+ Partials      872      870       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mandeep]

Hi @JeanChristopheMorinPerso!

On Windows 11 with a network drive formatted to NTFS, resolves were taking 45-60 seconds for any given project. There are 10 or so repos with 250-350 packages in each. This setup uses memcached with a hit rate of 99%. With this change, resolves are now 1-3 seconds. Calling os.listdir across a network for each package is extremely expensive, even with a memcached setup since the cache is unused.

If there's any command output or anything else you would like to see, please let me know!

[JeanChristopheMorinPerso]

I've only looked for 10 minutes, but so far, I'm not convinced that this will bring a performance improvement. In fact, I think this might actually create a performance regression. I think.

[JeanChristopheMorinPerso]

I've not yet spent enough time on this, but from a first look, self._get_family_dirs() calls os.isdir adn then os.listdir, and then for each dir returned by os.listdir, call os.isdir. This will result in a performance regression in theory... And _get_family_dirs is not cached, so I don't see how it can actually improve the performance.

[JeanChristopheMorinPerso]

Spent a little bit more time on this. The performance regression will only be when caching is disabled, which is the default.

[mandeep]

I am new to the codebase, but I think this line uses memcached for directory lookup.

rez/src/rezplugins/package_repository/filesystem.py

Line 549 in 6c3d754

self._get_family_dirs = decorator1(self._get_family_dirs)

You're correct that this would be a regression when caching is disabled. Ideally, os.listdir would not be called per package.

[mandeep]

@JeanChristopheMorinPerso, I was thinking that since the extra os.isdir call will slow down the default case even more that we could just add a check to see if the cache is enabled. If it is enabled then it runs the new cached branch, and if it is disabled then we can just run the old os.listdir check as is.

This can be achieved with a simple if statement or a more involved solution of a new decorator wrapped around listdir.

[maxnbk]

I'm 99% certain that this specific "fix" has come up before, and I do recall that it's fundamentally required for some mechanic reason in rez, but I'll need to hunt to come up with the reason why. That doesn't mean that opportunities for optimizing it, or perhaps finding creative ways to work around or configure-away the problem are without merit, but it does need scrutiny.

[JeanChristopheMorinPerso]

@maxnbk #689?

[mandeep]

Thanks @maxnbk. I agree that scrutiny is needed and hopefully we can work through a solution.

The fix here doesn't really alter much for those not using a cache. os.listdir() is still called to collect a list of directories. This will not change anything for case-insensitive filesystems I believe. The difference is that with this change, memcached can be used on Windows for this lookup. I believe this is already the case for case-sensitive filesystems.

In #689 it appears that the author added the os.listdir() call but did not provide a cached solution.