odoo / o-spreadsheet

Other
195 stars 44 forks source link

[FW][PERF] evaluation: early return errors #5254

Closed fw-bot closed 1 day ago

fw-bot commented 1 day ago

Description:

Throwing errors is super slow but the evaluation uses throw in many places when the formula result is an error (historic). Returning the error result is better than throwing.

Extensive work has been done in the past year to reduce throw, especially the "Loading..." errors.

But if a function receives an error object as an argument and the function uses one of the casting helpers (toNumber, toString, etc.) the error is still thrown.

With this commit, if a function argument is an error, we by pass the function compute and the error is directly returned.

That is unless the argument is declared as accepting the type any, in which case it can use the error argument as part of its business logic.

We currently only check simple scalar arguments (and not matrices) for performance reasons. Iterating over all matrices to check if there's any error is costly and probably outweights the benefits (especially when there isn't any error)

On one of the dashboard on odoo.com (id=116), the evaluation triggered by setting a global filter is reduced by ~70% (when setting a filter, everything is reloading and hence there are "Loading..." everywhere

before: 1707ms after: 533ms

There's no significant performance hit on regular evaluations (when there's no errors)

Many types of many function arguments had to be changed to remove any where the function doesn't need to handle errors itself.

For some function expecting an matrix of a given type (let's say range<number>), we added range<any>. The real type of those arguments is range<any> but from a functional POV, they expect range<number> and ignore any other values. We leave the range<number> only for an informative reason.

Task: 4328215

review checklist

Forward-Port-Of: odoo/o-spreadsheet#5163

robodoo commented 1 day ago

Pull request status dashboard

fw-bot commented 1 day ago

@LucasLefevre @laa-odoo cherrypicking of pull request odoo/o-spreadsheet#5163 failed.

stdout:

Auto-merging src/functions/index.ts
Auto-merging src/functions/module_database.ts
Auto-merging src/functions/module_logical.ts
Auto-merging src/functions/module_lookup.ts
Auto-merging src/functions/module_operators.ts
CONFLICT (content): Merge conflict in src/functions/module_operators.ts
Auto-merging src/functions/module_statistical.ts
Auto-merging src/types/functions.ts
Auto-merging tests/functions/functions.test.ts

Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).

In the former case, you may want to edit this PR message as well.

:warning: after resolving this conflict, you will need to merge it via @robodoo.

More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port

LucasLefevre commented 1 day ago

robodoo r+