Saturday, June 10, 2023
1 change · master
Code cleanup and technical improvements
This change reorganizes how Odoo caches website and web interface asset bundles such as JavaScript and CSS. It aims to reduce unnecessary cache duplication, speed up page loading after website changes, and make cache refreshes more targeted to lower the risk of broad performance disruptions.
Original PR description
## Warning If changing the ormcache keys and placement was the main goal of this pr, a `self.env['ir.qweb'].clear_caches()` was removed This is maybe the change that can have the must impact and may…
## Warning
If changing the ormcache keys and placement was the main goal of this pr, a `self.env['ir.qweb'].clear_caches()` was removed
This is maybe the change that can have the must impact and may create unexpected bug. It was replaced by more precise actions. See the corresponding **Remove cache invalidation** section for more info.
## Introduction
The current ormcache for assets bundle are not always _ideal_.
- Some cache key can have multiple values leading to the same result (example, lang defines rtl)
- Some cache key are useless (too many context key related to templates logic)
- Some cache key are not totally enough and works by "chance"
- Part of the computation stored in the ormcache is actually fast to generate (node attributes)
## 1. Improve _get_asset_content cache
### Move cache from _get_asset_content to _get_asset_paths
The `_get_asset_content` cache has many cache key that are related to a post-processing of the `_get_asset_paths` result. (node attributes)
`_get_asset_paths` can be slow because of file system access, but the post processing is actually just filtering files, generating node attributes for external assets, ....
Moving the cache to _get_asset_content will have the benefit to create less duplicates entries in the ormcache as well as less cache miss.
To simplify the `css` and `js` parameters are removed since they _only_ filter the output of get_paths, the heavy part of globing the file will be done before that. Anyway, they are both `True` when called from `_get_asset_content`, and the only other call, in `_get_related_bundle` don't really need to filter them since it is not a critical part regarding performance, and the result will remain the same.
### Better `_get_asset_paths` cache key
The initial orm cache key was using `_get_template_cache_keys`, a little overkill and creating duplicates entries again. The only context key needed is website_id for `_get_related_assets`.
The context key is actually not enough, the website_id depends on more parameters than that:
- request.session.get('force_website_id')
- request.httprequest.host
- existing websites
The idea here is to call `get_current_website` instead of using all parameters that could define the website.
In the same spirit of `_get_template_cache_keys` `_get_assets_params` can be overridden to give extra params that are useful to list assets path. Those params are computed before entering the method `_get_asset_paths`.
The only parameter here is the website_id.
Other parameter like `defer_load`, `lazy_load`, `media` are not useful since the cache was moved but this will actually change even further in next point.
### Result
Before:
```
@tools.ormcache('bundle', 'defer_load', 'lazy_load', 'media', 'tuple(self.env.context.get(k) for k in self._get_template_cache_keys())')
```
After
```
@tools.conditional(
'xml' not in tools.config['dev_mode'],
tools.ormcache('bundle', 'tuple(sorted(assets_params.items()))'),
)
```
The conditional part is actually to make it consistent with the assets_node, and enforce an expected result to discover new files in dev mode (was working before only by cache invalidation side effect) The final cache key is actually : (`bundle_name`, `website_id`)
## 2. Improve _generate_asset_nodes cache
The main reason of the orm cache is the slowness of the validation of the assets. This includes:
- listing files (_get_asset_content, see previous point)
- **computing version**
The cache key was, like the previous point, depending on
- `debug`
The only relevant value for debug is _contains assets_
We don't need to make the difference between debug='', debug='1', debug='test', and 'debug=assets', 'debug=tests,assets', ...
- `defer_load`, `lazy_load`,
Those values are only useful to generate html node, a leight-weight operation that does not really needs to be in cache.
The main idea to remove them from the ormcache key is simply to generate the nodes outside the ormcached values.
- `media`
The idea is the same as `defer_load` and `lazy_load`, but `media` was also used in the generation and it looks useless if we have the media on the node. Since media is not used to generate the attachment url, it doesn't make sence to use it in the generation. Also, no t-call-asset with a media= was found in the master code base.
- `async_load`
This one is similar to `defer_load` and `lazy_load` but it looks like it wasn't used anymore. This was simply removed.
- context.get('lang')
The only information needed is the direction, rtl or ltr. This means en and fr languages, despite sharing the same css assets, will duplicate the ormcache entries.
- `_get_template_cache_keys`
Only the lang and webiste where really relevant in this flow. Other keys are actually useless in this flow.
As explained in the previous point, some information used in the generation where not in the orm cache key
- `self.env.user.lang` if there is no lang in the context
- `request.session.get('force_website_id')`
- `request.httprequest.host`
- ...
The proposed solution is to:
- extract any information needed from the context, request, environment before entering the ormcache, reduce it to the minimal possible set of values needed
```
rtl = self.env['res.lang']._lang_get_direction(self.env.context.get('lang') or self.env.user.lang) == 'rtl'
assets_params = self.env['ir.asset']._get_assets_params() # website_id
debug_assets = debug and 'assets' in debug
```
and remove a leightweight part of the logic
```
def _get_asset_nodes(self, bundle, css=True, js=True, debug=False, defer_load=False, lazy_load=False, media=None):
links = self._get_asset_links(bundle, css=css, js=js, debug=debug)
return self._links_to_nodes(links, defer_load=defer_load, lazy_load=lazy_load, media=media)
```
Where _get_asset_links is the heavy cached part, and _links_to_nodes is the lightweight part generating the nodes based on the `defer_load`, ....
Additional notes:
- data-asset-version and data-asset-bundle are removed from the node since they don't seem to be used anymore since 65d70acdbfb1027f0937b5af7c4f3e9a571a4c6e
- async_load is removed since there is no occurence of this in the code.
- a small hack is still needed to pass javascript content instead of links, this is only to manage css compile error and will hopefully be removed in the future.
- a context key is still in use to generate the bundle, the `commit_assetsbundle` but it has no impact on content and will hopefully be removed in the future.
## Add test for ormcache hit/miss
In this context, hit/miss is about having the same cache key for the same result. This test demonstrates the current state, were entries are create in the ormcache only if the key is really different and will lead to a different result.
## Remove cache invalidation
This cache invalidation is quite aggressive since every time an asset bundle is updated, all workers will clear their cache.
The concerned cache by this clear_cache is `_generate_asset_nodes_cache` trough `_get_asset_nodes`.
The cache is ignored, both in dev=xml and debug=assets.
This clear cache was made conditional in https://github.com/odoo/odoo/commit/553ea82f8135454b90888bf200372744729d7b20 but this does not solve an issue we can have in production.
Lets imagine a clean starting state
- all sources are updated
- all workers are restarted.
The orm caches are all empty, but since the sources changed, all bundles will be recomputed. This means that every bundle updated in database with save_attachement will invalidate the cache of all workers. Rendering a pdf report of any kind using a specific bundle will invalidate all cache. Starting a debug=assets for the first time will invalidate all cache, even if the cache is not used in this case.
But for a regenerated bundle we would expect the ormcache to be:
- empty (did not generate the same bundle yet)
- have the same value (concurrent generation of the same bundle)
Having a different value would mean that the bundle was generated with another version of the sources. In this case it is maybe even better not to invalidate the cache since it could lead to an invalidation war between two workers.
The only case where invalidating this cache is useful is when a bundle changes, Usually if an ir_asset is created, modified, ...
There is still another rare but possible possibility to have a 404 if the transaction is roll backed after populating the assets node cache.
In this case, we only need to clear the cache locally in case of rollback.
## Copy bundle when possible (2nd commit)
If get_attachments fails, the next step will be to generate the attachments from scratch, a slow operations.
When creating a new website, all assets bundle would actually be similar to their version without website, but the url is different.
This can be visible because the first loading of /web is slow after creating a new website: the website_id is forced in the session and the `assets_backend` are regenerated, identical to the original ones.
This commit proposes to try to find an attachments with different extra but the same uniquifier when possible and copy it's content.
Note that just returning the other attachment url may work, but it would be confusing to randomly have links to assets coming from another website_id. This would also be a problem if the original attachment is unlinked, forcing to recompute it for other websites.
Good to now, since the content is the same, no duplication of the content should appear in the file store, just an entry in the database.
## Third commit
Because of all previous changes, we can simplify even futher. The main idea is to avoid to be forced to call `_get_asset_content` before `_get_asset_bundle` (or `AssetsBundle`). We can get rid of `AssetsBundleMultiWebsite` by making the small change of behaviour between the two work by calling an `ir.asset` method `_get_asset_extra` to generate the extra parameter. This in fine allows to have less overrides.
The "remains" (external assets url) of get_assets_content are also stored on the assets_bundle.