-
Notifications
You must be signed in to change notification settings - Fork 31
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
perf: avoid using exceptions for flow control #1074
Conversation
Signed-off-by: Michael Beemer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change! Even though this is more a "stylistic" change than a performance benefit.
It should have some minor performance improvements but I need to run proper performance tests to prove it. 😄 Here's a fun snippet from Stack Overflow to see how impactful throwing an error is on performance. import { performance } from "perf_hooks";
const error = new Error("invalid exception");
function ThrowException() {
throw error;
}
const _ATTEMPT = 1000000;
function ThrowingExceptions() {
const p1 = performance.now();
for (let i = 0; i < _ATTEMPT; i++) {
try {
ThrowException();
} catch (ex) {
// log error
}
}
const p2 = performance.now();
console.log(`ThrowingExceptions: ${p2 - p1}`);
}
function ReturnException() {
return { error };
}
function ReturningExceptions() {
const p1 = performance.now();
for (let i = 0; i < _ATTEMPT; i++) {
const ex = ReturnException();
if (ex.error) {
// log error
}
}
const p2 = performance.now();
console.log(`ReturningExceptions: ${p2 - p1}`);
}
function Process() {
ThrowingExceptions();
ReturningExceptions();
ThrowingExceptions();
ReturningExceptions();
}
Process(); And here are the results on my computer. ThrowingExceptions: 365.008125 |
Oh this is interesting @beeme1mr. I thought for V8 this would not affect the performance, had it in mind from some discussion I had in the past.
When I inline the error creation, I consistently get faster times with the throw (and sure long times due to Error creation overhead):
import { performance } from 'perf_hooks';
const _ATTEMPT = 1000000;
function ThrowingExceptions() {
const p1 = performance.now();
for (let i = 0; i < _ATTEMPT; i++) {
try {
throw new Error('invalid exception');
} catch (ex) {
// log error
}
}
const p2 = performance.now();
console.log(`ThrowingExceptions: ${p2 - p1}`);
}
function ReturnException() {
return new Error('invalid exception');
}
function ReturningExceptions() {
const p1 = performance.now();
for (let i = 0; i < _ATTEMPT; i++) {
const ex = ReturnException();
if (ex) {
// log error
}
}
const p2 = performance.now();
console.log(`ReturningExceptions: ${p2 - p1}`);
}
function Process() {
ThrowingExceptions();
ReturningExceptions();
ThrowingExceptions();
ReturningExceptions();
}
Process(); |
What version of node are you using? Perhaps performance has improved in recent versions. 🤔 |
Tried Node 16, 20 and 22, all the same, and my Chrome so several V8 versions, I guess it simply is notable overhead when testing it in the synthetic benchmark. I guess in reality the error creation alone compensates the performance benefit, but as we are not creating errors here anymore I guess we can actually have a little performance improvement :) |
🤖 I have created a release *beep* *boop* --- ## [1.3.2](web-sdk-v1.3.1...web-sdk-v1.3.2) (2024-11-07) ### 🐛 Bug Fixes * update OpenFeature core version to 1.5.0 ([#1077](#1077)) ([a3469b6](a3469b6)) ### 🧹 Chore * loosen peer dependency requirements, remove some ci automation ([#1080](#1080)) ([ef3ba21](ef3ba21)) ### 🚀 Performance * avoid using exceptions for flow control ([#1074](#1074)) ([26264d6](26264d6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [1.16.2](server-sdk-v1.16.1...server-sdk-v1.16.2) (2024-11-07) ### 🧹 Chore * loosen peer dependency requirements, remove some ci automation ([#1080](#1080)) ([ef3ba21](ef3ba21)) ### 🚀 Performance * avoid using exceptions for flow control ([#1074](#1074)) ([26264d6](26264d6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <[email protected]> Signed-off-by: Todd Baert <[email protected]> Co-authored-by: Todd Baert <[email protected]>
This PR