Master formula function ref assert and errors laa #6248
Conversation
|
|
laa-odoo
force-pushed
the
master-formula-function-ref-assert-and-errors-laa
branch
11 times, most recently
from
c396d46 to
bff4dac
Compare
|
I apologize if I missed something obvious, I justquickly went over the diff. Is there a reason why we keep some asserts in given situations? If so, it could be worth mentioning it somewhere (commit message and/or codebase if relevant?) unrelated to the content but still worth mentioning:
|
Initially, the compute time spent handling errors in formulas was unusually high.
At the same time, we had a unification problem: in all the code that implements formulas, we had several ways to handle errors:
Note that there is also a technical constraint: it is not possible to use To try to fix both issues, a first proposal was made in this PR #6089:
After that proposal, it was decided that we want to maximize performance even if it means giving up unification. So we chose to return errors instead of throwing/asserting where possible (in the compute functions), and we keep throwing/asserting in the helper functions. Now, the question you raise is interesting. Since there are only about thirty |
|
@rrahir Thanks for comments on the commit message :) |
laa-odoo
force-pushed
the
master-formula-function-ref-assert-and-errors-laa
branch
from
bff4dac to
13a43cc
Compare
LucasLefevre
force-pushed
the
master-formula-function-ref-assert-and-errors-laa
branch
from
13a43cc to
2bde477
Compare
src/functions/helper_assert.ts
Outdated
| if (args.some((arg) => Array.isArray(arg) && (arg.length !== 1 || arg[0].length !== 1))) { | ||
| throw new EvaluationError(errorStr); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
return !args.some((arg) => Array.isArray(arg) && (arg.length !== 1 || arg[0].length !== 1))
For performance reasons: - in compute functions, replace asserts with return errors - in asserts, replace lambda condition by normal condition Initially, the compute time spent handling errors in formulas was unusually high. This happened mainly for two reasons: - Creating errors from native JavaScript errors is very costly. - Throwing errors is also costly (but still less than the first point). At the same time, we had a unification problem: in all the code that implements formulas, we had several ways to handle errors: - Using throw with the error we want - Using return with the error we want - Using assert, which throws specific errors (more elegant than a simple throw after a condition) Note that there is also a technical constraint: it is not possible to use return error in the helpers that formulas use. Helpers only can use throw and assert. To try to fix both issues, a first proposal was made in this PR #6089: We fixed the heavy performance cost by avoiding creating errors based on native JS errors. We unified error handling into a single method: using assert. After that proposal, it was decided that we want to maximize performance even if it means giving up unification. So we chose to return errors instead of throwing/asserting where possible (in the compute functions), and we keep throwing/asserting in the helper functions. Co-authored-by: Anthony Hendrickx (anhe) <[email protected]> Task: 4760596
LucasLefevre
force-pushed
the
master-formula-function-ref-assert-and-errors-laa
branch
from
2bde477 to
5dd6bec
Compare
|
robodoo r+ |
For performance reasons: - in compute functions, replace asserts with return errors - in asserts, replace lambda condition by normal condition Initially, the compute time spent handling errors in formulas was unusually high. This happened mainly for two reasons: - Creating errors from native JavaScript errors is very costly. - Throwing errors is also costly (but still less than the first point). At the same time, we had a unification problem: in all the code that implements formulas, we had several ways to handle errors: - Using throw with the error we want - Using return with the error we want - Using assert, which throws specific errors (more elegant than a simple throw after a condition) Note that there is also a technical constraint: it is not possible to use return error in the helpers that formulas use. Helpers only can use throw and assert. To try to fix both issues, a first proposal was made in this PR #6089: We fixed the heavy performance cost by avoiding creating errors based on native JS errors. We unified error handling into a single method: using assert. After that proposal, it was decided that we want to maximize performance even if it means giving up unification. So we chose to return errors instead of throwing/asserting where possible (in the compute functions), and we keep throwing/asserting in the helper functions. closes #6248 Task: 4760596 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Co-authored-by: Anthony Hendrickx (anhe) <[email protected]>
Task: 4760596