1637820 - Make mozbuild.util.memoize support memoizing functions with kwargs

Status: REOPENED

(Firefox Build System :: General, enhancement, P5)

Description

[Marco Castelluccio [:marco]]

Similarly to https://github.com/mozilla/adr/pull/120, we should make mozbuild.util.memoize (https://searchfox.org/mozilla-central/rev/9f074fab9bf905fad62e7cc32faf121195f4ba46/python/mozbuild/mozbuild/util.py#996) support memoizing functions with kwargs.

Comment 1

[Ricky Stewart]

Note that Python 3 has built-in memoization in the standard library, so augmenting the homebrewed memoize function is probably not a good use of time.

Comment 2

[Marco Castelluccio [:marco]]

True, the only problem with the built-in lru_cache function is that it leaks memory when applied to a class method. So we'd need to be careful with it.

Comment 3

[Mitchell Hentges [:mhentges] 🦀]

True, the only problem with the built-in lru_cache function is that it leaks memory when applied to a class method. So we'd need to be careful with it.

Ah, I'm guessing that that lru_cache problem is this python ticket here.
FWIW, this same downside applies to our homebrewed memoize function, too.

Comment 4

[Mitchell Hentges [:mhentges] 🦀]

Since we're now running with Python 3.6+, we can lean on the built-in lru_cache to memoize kwargs functions.

Comment 5

[Mike Hommey [:glandium] OoO until Jun 18]

Should we keep this open to make memoize = functools.lru_cache(maxsize=None)? (3.9 has functools.cache but 3.9 is a long way)

Comment 6

[Mitchell Hentges [:mhentges] 🦀]

I'm not sure that having our own alias is necessary.
However, I would be on-board with migrating existing mozbuild.util.memoize usages to instead use lru_cache.

Comment 7

[Mike Hommey [:glandium] OoO until Jun 18]

The point of an alias is that functools.lru_cache is not very convenient replacement on its own. functools.cache would be.

Comment 8

[Mitchell Hentges [:mhentges] 🦀]

Sure, that would work.

Comment 9

[Bob Hood [:bhood]]

So, is the hand-rolled memoize class in utils.py still in use? If we have moved on to the built-in in python3, perhaps the util.py version should be removed from the code, this bug closed, and a different bug logged for the suggested alias solution above?

Comment 10

[Alex Hochheiden [:ahochheiden] [he/him]]

Yeah, since 3.9 is our minimum supported version for a while now (soon to be 3.10) we should be able to replace this with the builtin functools.cache and functools.cached_property.