odoo#254660 seems to solve part of the problem. the problem was much worse before that.

@huguesdk

I'd like to give my 2 cents on this, but please note that this is my personal opinion and I don't speak on behalf of Odoo. In my opinion, I do believe this approach is better instead of attempting a preload of the entire cache of statics. Avoiding the expensive os.listdir is indeed a good move. However, acquiring a lock even on cache hits is a bit of an overkill imho since this cache population is idempotent and harmless.

Also, I don't think removing the default argument value of _get_manifest_cache() makes any difference to the built-in, functools.lru_cache. Below is a test confirming that having a default None value for an argument doesn't impact the caching mechanism.

In [13]: @functools.lru_cache(maxsize=None)
    ...: def test_lru_cache(x, y=None):
    ...:     return random.randint(0, 100), x, y
    ...: 

In [14]: test_lru_cache(1)
Out[14]: (921, 1, None)

In [15]: test_lru_cache(1)
Out[15]: (921, 1, None)

In [16]: test_lru_cache(1, 10)
Out[16]: (311, 1, 10)

In [17]: test_lru_cache(1, 10)
Out[17]: (311, 1, 10)

Unfortunately though, such PR is very unlikely to be accepted in stable version since we try to minimize changes on that level unless it's absolutely necessary and we can find exact examples where this change would make a significant difference.

what you guys should do is try to get such things merged in more recent Odoo version (the supported ones 17, 18 or 19) and then we could eventually backport it. Also to get such a thing merged into odoo/odoo, you better target 19 or master but don't fool yourself, the likehood to get somebody at Odoo look at a community PR is extremely low anyway, sadly. But I mean if there is a way this is the way...

@MohamedKasem99 @rvalyi thank you for your answers and taking the time to look into this.

about the thread lock, i was hesitant, as i read somewhere that not all operations on dicts are thread safe, so i added it to be sure, especially since a lock like this is quite cheap (functools.lru_cache() uses an RLock (which is slightly more expensive) for each call, for instance), especially compared to reading and serving a static file. but i’ve just read here that it is indeed safe to modify a dict from multiple threads, so i’ve removed it. thanks for the heads-up!

about _get_manifest_cache(), you are right about the fact that functools.lru_cache() works fine with default arguments. however, calling the function with a different number of arguments will result in multiple cached values, even if the arguments have the same value as the default value, as functools.lru_cache() doesn’t know about the default value.

with your example function:

>>> test_lru_cache(1)
(21, 1, None)
>>> test_lru_cache(1)
(21, 1, None)
>>> test_lru_cache(1, None)
(51, 1, None)
>>> test_lru_cache(1, None)
(51, 1, None)

the problem here comes from the fact that get_manifest() always calls _get_manifest_cache() with 2 arguments (the second being None if mod_path is not provided, which is always the case), while in ir_asset.py, there are calls to _get_manifest_cache() with only 1 argument.

because of this, we end up with 2 cached values for each module in _get_manifest_cache(): one for the call with only 1 argument (only in ir_asset.py), and one for the call with 2 arguments (even if the 2nd is None) (in the rest of the code).

@rvalyi

I can't really speak for the reasons behind the delays on merging community PRs. I try to look at the ones I get a chance to but I'm not sure about other teams priorities at the moment. The changes made in my original PR have been forward ported to more recent Odoo versions so that part at least is already fixed in the current stable versions.

@huguesdk Yes you're right actually I must've gotten the implementation wrong of the lru_cache in python. In this case I believe your edits on that part are indeed correct and should be kept. However, I'm not sure if it can be accepted in stable versions. The edit I made in my PR was extremely minimal so I can get it merged but this one although it's more complete it might need much more convincing. In any case, thanks a lot for pointing this out to me!