-
Notifications
You must be signed in to change notification settings - Fork 207
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
test performance upgrade #523
base: master
Are you sure you want to change the base?
Conversation
Thanks, @cortopy! I should have time to look at this over the weekend. |
Quick, question about how you ran these. It's best to run them with as little else running on the machine as possible. There's really no way to get a "perfect" run (whatever that might mean), of course, but at least on OS X I've seen some weird outliers when I've forgotten to quit everything but a terminal window. So, if you did that, great! This looks ready to merge. If not, would you mind doing another run? Thanks! Also, if you're up for it, I'd be delighted to have a similar PR over at |
I did close all windows for the reasons mentioned. But that's a good point. I'll try again by restarting my computer in terminal mode |
Thanks, @cortopy, I appreciate the extra effort. I've not actually booted into console mode to do it, but usually quit all user apps. It'll be kinda cool to see what differences dropping to console mode reveals! |
@briancavalier interestingly, not much of a difference in terminal mode. I've done a new commit with the results |
Awesome, thanks @cortopy. Now we know :). I guess operating systems (perhaps particularly linux) have gotten pretty good at not using resources when idle. I'll give the code another close look before merging. |
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.
Looks great overall. Just one comment about merge-nested.
for(var i=0; i<n; ++i) { | ||
s.push(create(a)); | ||
} | ||
return create(a).pipe(rxjs6Operators.merge(...s)); |
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.
I don't think this is testing the same intent, and I apologize if that intent wasn't clear! The intent is specifically to merge pairs of streams, rather than to rely on a library's bulk merge operator.
The reason is that across a large application, you may not tend to have all the streams in your hand at once. For example, you might call a function which, unbeknownst to you, internally merges 2 streams, and returns the result to you, and you then merge that with another stream, and so on.
MostJS focuses not only on performance, but performance scalability, ie how does it perform as usage scales without the dev having to think about it. For example, MostJS can (and does) optimize the situation described above.
So, I think rx 6 can reuse the merge
helper function above, and provide a small wrapper lambda around rxjs6.from
as create
, similar to what the rx 5 tests do.
Does that make sense?
Summary
New performance tests are in!!
Major changes:
Todo
Compare most.js with the latest versions of other reactive frameworks