Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] website,* : reduce number of sql queries on generic code #48165

Closed
wants to merge 12 commits into from

Conversation

@rdeodoo
Copy link
Contributor

rdeodoo commented Mar 23, 2020

Reduce number of queries on base method like dispatch, or base controller like /web/content

Part of task-2211013

Following
#46925
#47257

Most of these small queries are done in part of code called a lot of times.
Next step is to be more 'controller' specific, and reduce query / imp perf
for specific page /my /blog /forum /shop ...

Courtesy of Rco and AL for help and validation

Co-authored-by: Jérémy Kersten jke@odoo.com
Co-authored-by: Romain Derie rde@odoo.com

@C3POdoo C3POdoo added the RD label Mar 23, 2020
@robodoo robodoo removed the CI 🤖 label Mar 23, 2020
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch 2 times, most recently from 544386e to 2372239 Mar 24, 2020
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch 2 times, most recently from 8fb77b6 to 1cb36a6 Mar 24, 2020
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch 3 times, most recently from eeb0f50 to 87bdef7 Mar 24, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 25, 2020
@JKE-be JKE-be force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch from 5442c97 to ccb796b Mar 26, 2020
@robodoo robodoo added the CI 🤖 label Mar 26, 2020
@JKE-be JKE-be force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch from ccb796b to cf0406a Mar 26, 2020
@robodoo robodoo removed the CI 🤖 label Mar 26, 2020
addons/website/tests/test_performance.py Show resolved Hide resolved
addons/website/models/ir_http.py Outdated Show resolved Hide resolved
addons/website/controllers/main.py Show resolved Hide resolved
addons/website/controllers/main.py Show resolved Hide resolved
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch 3 times, most recently from 2293edf to c0296dc Mar 26, 2020
@robodoo robodoo added the CI 🤖 label Mar 26, 2020
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch from c0296dc to db16cc0 Mar 26, 2020
@robodoo robodoo removed the CI 🤖 label Mar 26, 2020
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch 2 times, most recently from 37d56fb to c84bdc7 Mar 26, 2020
@antonylesuisse

This comment has been minimized.

Copy link
Contributor

antonylesuisse commented Mar 26, 2020

r+

When creating a website, we:
  1. Create the website
  2. Bootstrap a homepage
  3. Copy the generic menu hierarchy tree for that website

But since we just bootstraped a homepage for the website, the home menu was not
using that page but the generic one from the xml data instead.

That's an issue for tests recently introduced in master that needs the relation
to be correctly set for the query count to work as expected.

task-2211013
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch from df5ce2a to 20a8de6 Mar 26, 2020
rdeodoo and others added 11 commits Mar 26, 2020
  - Refactor for easier readability
  - Add a test to ensure menu hierarchy scale correctly.
  - Add tests for image controllers
  - Add test for assets controllers

task-2211013
Render page flow:
  - Search website.page in _serve_page for requested URL
  - Check if the page is visible with `is_visible`
  - `is_visible` will get the `ir.ui.view` of the page
    Note that this is not 'cachable' as being visible depends of now() for the
    publishing date
  - Render layout which is getting the website menus
    Note that this can't be cached either as a menu visibility might also
    depends of it's website.page visibility which depends of now()
  - Checking menus `is_visible` fetch the menus pages and those pages views.

Before this commit, every level of menu hierarchy would add 2 queries.
This can be avoided by prefetching everything in once during the dispatch if we
know we will render the website menu.
In such a case, read `is_visible` on every website menus will fetch every
pages, views and menus in the lowest SQL queries possible, eg one query for
every table.

For a 3 level menus, requests goes from 20 to 12 to load a untracked page.

task-2211013
Before this commit, the cache would always been cleared even if not needed.
Now, we only clear cache when needed.

Util:
Introduced with 9920f20#diff-1407a8ce197a04eefaefa26c127a4418R327
Changed with 0d5fcdc

task-2211013
If we are rendering a page, the menus will most likely be rendered as well.
Note that even in a 403 case when the page is found but is not visible, the
menus will also be shown on the 403 page.

Fetching the menus before accessing the requested page will prefetch that page
as well in one go if that page is in the menus, without any costs for the pages
not in the menus.

There is a tradeoff for 'non-layout' pages, which only occurs in advanced
technical cases:
1. Create a page with specific extension as name suffix (page.css) in which
   case the page will be bootstraped accordingly, without call to layout.
2. Remove the call to layout in HTML editor or backend
3. Remove the call to submenus template in HTML editor or backend
For those cases, the menus will be prefetched for no reason.

Note that homepage '/' is rendered through a controller, same logic is applied
there.

task-2211013
Most of the time, the record exists, but we still ensure it does, every single
time, making an SQL query.
Doing a try catch will result in the same behavior, but won't make that query
most of the time.

task-2211013
Was introduced with bae7e32 for `website_version` which doesn't exist
anymore.

task-2211013
If model is already an ir.attachment we don't need to do a new search_read.
We can use it record directly.

After this commit, we don't do extra request if we already have the info,
else we don't change the behavior.

task-2211013
Note that this route is only used when no icon is set in DOM, such as accessing
a PDF after an order.

task-2211013
In case of short controller that don't use qweb template, we don't need
anyhting else that this url code that don't change frequently.

It make only sense for model like lang, website, ... that will not change
frequently. And are called on each call by the dispatcher.

In case of a website page, we will btw browse lang later, but in case of
small controller like /favicon.ico, or page without qweb, ... we can just
use the same from last query.
The dispatching layer of Odoo (common to any call) can avoid a query on website
if those 3 values are cached.
Note that for complex business flows, there won't be any gain since website
infos will have to be fetched at some point.

About user_id:
In most cases, for a website page you will need to browse the website btw so
we don't remove this request previously. But in case of a simple image, the
controller /web/content need the public user just to set the request.uid in
auth_public but no other info from website.

With this commit, we don't do the 'select * from website where id in()'
request on each controller declared in auth='public' but use the user_id
from the cache. (Invalidated by the write on website_id)

task-2211013

Co-authored-by: Jérémy Kersten <jke@odoo.com>
Co-authored-by: Romain Derie <rde@odoo.com>
@rdeodoo rdeodoo force-pushed the odoo-dev:master-imp-sql-query-page-jke-rde branch from 20a8de6 to 7dd57a1 Mar 26, 2020
@JKE-be JKE-be changed the title Master imp sql query page jke rde [IMP] website,* : reduce number of sql queries on generic code Mar 26, 2020
@JKE-be

This comment has been minimized.

Copy link
Contributor

JKE-be commented Mar 26, 2020

@robodoo r+ rebase-merge

@robodoo robodoo added the r+ 👌 label Mar 26, 2020
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 26, 2020

Merge method set to rebase and merge, using the PR as merge commit message

@robodoo robodoo added the CI 🤖 label Mar 26, 2020
robodoo added a commit that referenced this pull request Mar 26, 2020
Reduce number of queries on base method like dispatch, or base controller like /web/content

Part of task-2211013

Following
    #46925
    #47257

Most of these small queries are done in part of code called a lot of times.
Next step is to be more 'controller' specific, and reduce query / imp perf
for specific page /my /blog /forum /shop ...

Courtesy of Rco and AL for help and validation

closes #48165

Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
Co-authored-by: Jérémy Kersten <jke@odoo.com>
Co-authored-by: Romain Derie <rde@odoo.com>
@robodoo robodoo closed this Mar 26, 2020
@robodoo robodoo deployed to merge Mar 26, 2020 Active
@rdeodoo rdeodoo deleted the odoo-dev:master-imp-sql-query-page-jke-rde branch Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.