-
Notifications
You must be signed in to change notification settings - Fork 3k
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(every): remove tryCatch/errorObject (~1.5x improvement) #1266
Conversation
if (result === errorObject) { | ||
return new ErrorObservable(errorObject.e, source.scheduler); | ||
} else { | ||
try { |
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.
You should isolate the try/catch to it's own function. Think of it this way: All code that is in the function that contains the try/catch is unoptimized... so you want most of the code to live where try/catch does not.
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.
Got it. I'll do this and update the PR.
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.
In this operator, we have performance improvements only for the common case (non-scalar/array). Both removing tryCatch()
from the particular cases only, and removing it plus moving the try/catch blocks to separate functions changes almost nothing in terms of performance.
What should we do in cases such as this one? Remove the tryCatch()
anyway or leave as it is now in the codebase?
this.notifyComplete(false); | ||
try { | ||
const result = this.predicate.call(this.thisArg || this, value, this.index++, this.source); | ||
if (!result) { |
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.
you don't want to notifyComplete
instead of this try/catch block. If it errors in the completion handler, it will send the error down the error path, which isn't the correct behavior.
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.
Makes sense. I'll move the notifyComplete
call to outside the try/catch block.
959a596
to
b745978
Compare
Just updated this PR. |
b745978
to
bb1f1d4
Compare
@Blesh Done. |
Before: | RxJS 4.0.7 | RxJS 5.0.0-beta.1 | factor | % improved ------------------------------------------------------------------------------------------------ every-predicate-array - immediate | 94,171 (±0.40%) | 294,037 (±0.33%) | 3.12x | 212.2% every-predicate-scalar - immediate | 112,027 (±0.59%) | 731,953 (±0.38%) | 6.53x | 553.4% every-predicate-this - immediate | 41,073 (±0.43%) | 160,923 (±0.38%) | 3.92x | 291.8% every-predicate - immediate | 44,075 (±0.43%) | 159,303 (±0.37%) | 3.61x | 261.4% After: | RxJS 4.0.7 | RxJS 5.0.0-beta.1 | factor | % improved ------------------------------------------------------------------------------------------------ every-predicate-array - immediate | 91,414 (±0.91%) | 418,446 (±0.31%) | 4.58x | 357.8% every-predicate-scalar - immediate | 112,075 (±1.06%) | 907,408 (±0.19%) | 8.10x | 709.6% every-predicate-this - immediate | 42,068 (±0.42%) | 295,388 (±0.23%) | 7.02x | 602.2% every-predicate - immediate | 44,054 (±0.39%) | 285,513 (±0.24%) | 6.48x | 548.1%
bb1f1d4
to
6cb1457
Compare
Remove blocked label, change looks ok to me. |
Merged with 14afeb6... thanks @luisgabriel! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Before:
After: