benchmark: complete the sqlite is-transaction fix - #65218
Conversation
Sorry, something went wrong.
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
|
Review requested:
|
Sorry, something went wrong.
H4ad
left a comment
There was a problem hiding this comment.
The initial idea was to assert this never returns invalid value due any optimization (such as fast apis)
But this works as well, thanks for the fix
Sorry, something went wrong.
No prob. Btw, this is my first time changing benchmark stuff. Do we need CI for it? |
Sorry, something went wrong.
|
Benchmark GHA (sqlite / sqlite-is-transaction): https://github.com/nodejs/node/actions/runs/31584406184 Results
Benchmark results: confidence improvement accuracy (*) (**) (***) sqlite/sqlite-is-transaction.js transaction='false' n=10000000 0.06 % ±2.36% ±3.11% ±3.99% sqlite/sqlite-is-transaction.js transaction='true' n=10000000 -0.62 % ±2.38% ±3.13% ±4.02% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results: 0.10 false positives, when considering a 5% risk acceptance (*, **, ***), 0.02 false positives, when considering a 1% risk acceptance (**, ***), 0.00 false positives, when considering a 0.1% risk acceptance (***)
|
Sorry, something went wrong.
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com> PR-URL: #65218 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com> PR-URL: #65218 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This PR removes the Logical AND assignment. It was causing the benchmark to call the isTransaction only once when the getter was returning false.
With the deadCodeElimination being false, the right side never runs.
Before
After