-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
events: improve performance for emit(), on(), and listeners() #13155
Conversation
I totally get the why of this change, especially given the performance regressions we've seen in events lately that will hopefully be fixed once the v8 patch lands... but, I can't say I'm too thrilled with this change because of the increased complexity of the function calls. |
@jasnell If you're referring to the As far as reducing code, there's not much that can be done without hindering performance, except compile-time macros, but those have their own problems. |
benchmark/events/ee-emit.js
Outdated
@@ -8,6 +8,7 @@ function main(conf) { | |||
var n = conf.n | 0; | |||
|
|||
var ee = new EventEmitter(); | |||
ee.setMaxListeners(listeners + 1); |
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.
Where is listeners
defined?
Edit: nvm, I only watched first commit.
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.
Fixed.
I'm not a fan of code duplication but the performance improvements justify this. |
Can you try this with Node with V8 60? I wonder if we've fixed the performance glitches and the loop unrolling. /cc @nodejs/v8 |
No arguments pass to emit() in the test? |
+1. Since 8.x is planning to ship with TF + I we should likely only land this if there is a persistent perf improvement seen on the new toolchain |
Did you try with TurboFan? This looks a lot like hardcore CrankshaftScript to me. |
@bmeurer I did actually try (without these changes and with variations on these improvements) with |
As of 5.8, yes, regressions would be expected. There is a test build available that uses 6.0 that should be tested against. |
Here are the sources for Node with V8 6.0 (V8 master). If you tell me how to run the benchmark I can try later today. Edit: Use this I'm not in favor of these changes:
I suggest we check with V8 60, if there's still a performance difference. If there is, we'll investigate on the V8 side if we can help at all. If we do end up landing this, I'd like meaningful comments on each loop explaining the change. And a commitment from somebody to revisit this every time we update v8. Preferably a script that runs the benchmark with and without these patches. (This might be overly ambiguous). |
And here are the sources for Node (master) with V8 6.0 (master) 😉. |
@fhinkel Regarding "stale" optimizations, that's most likely always going to be the case. It's very much a cat and mouse game because V8 (and other VMs for that matter) is changing all the time to varying degrees and we do not always upgrade to the latest major V8 version as soon as they go stable. Something else to consider is that while optimizations may go "stale" in master, they could very well still be applicable for older branches that do not get major V8 upgrades. As far as VM neutrality goes, there is no real good answer for that AFAIK. We cannot just create copies of parts of node for different VMs if we ever want to do optimizations. Has there been any discussion about this in any of the working groups or VM "meetups?" I don't think we have the resources to run benchmarks regularly before and specific changes. We've made a lot of (and probably will in the future) changes that may/may not be V8-specific and trying to manage and test all of those would probably be a nightmare (not to mention how some of the changes may interact with each other...). |
Maybe you dont understand what I mean (or I dont know something).
only case 1 will run, and cases 2, 3, 4 will never. |
@simonkcleung There is a separate benchmark for multiple arguments. The two could probably be merged together at some point I suppose. However the performance improvements should be similar for those other cases. |
fn[3].call(this); | ||
if (fnlen === 4) break; | ||
fn[4].call(this); | ||
if (fnlen === 5) break; |
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.
if (fnlen === 5) [](start = 7, length = 17)
Is there any significance of unrolling the loop for 5
? Or is it that in most common scenario there are not more than 5 event handlers to trigger? Either case, could you write a comment about it?
@jasnell - Could you please elaborate what v8 patch you are mentioning? Were there changes in inline heuristics of v8 that regressed events? |
@kunalspathak ... the patch was a performance improvement to Array.prototype.shift/unshift. I don't have the specific PR handy, but it landed in core and the performance regression I was seeing appears to have been addressed. |
Can we check it against streams and/or http benchmarks? I checked some basic benchmarks I typically use to check new node releases, and I am not seeing any significant performance improvements with this branch. |
It seems using rest parameters is better in both readability and performance in current v8 used by node ver. 8.0.0. |
Ok I just re-tested this PR as-is with V8 5.9 which is now in master and just from a few runs the trend I'm seeing is this:
|
Any further comments on this, given the performance details I previously posted for V8 5.9 (which uses TurboFan by default now)? |
Results:
CI: https://ci.nodejs.org/job/node-test-pull-request/8230/
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)