-
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
chore(DeferObservable): add benchmarks & minor perf #1393
Conversation
result.subscribe(subscriber); | ||
} | ||
} | ||
} | ||
|
||
function callObservableFactory<T>(factory: () => Observable<T>, errorSubscriber: Subscriber<T>): Observable<T> { |
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.
Can you try adding this as a member of DeferObservable? That way you don't create a closure in _subscribe
to get at callObservableFactory
. It probably won't make a huge difference, but I wanted to bring it up.
okay. there is no regression after doing it.
|
I updated this pull request and the description. |
@saneyuki, can you flatten the last two commits? |
Adding "Blocked" until the last two commits are flattened, Then it's good to go. Attn @kwonoj |
I'll keep eye on this. 👀 |
@kwonoj squashed! |
I tried this before check in, seeing results as below
I've been experienced sometimes number shows differently on my machine only and maybe this is same cases, but would like to confirm once again. @saneyuki , would you mind provide number again with latest changes? |
This is the newly benchmark result (the based master is ec4f2b7) Environment
ResultBefore
After
|
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. |
This is the benchmark result on Node v5.9.0,
MacBook Pro (Retina, 15-inch, Mid 2014, 2.8 GHz Intel Core i7, 16 GB 1600 MHz DDR3).
Before (7116d1d)
After changeset 09cd8d0
After changeset 70c3247