timers: improve linked list performance #8961
Conversation
Trott
added
the
timers
|
Benchmark results: (Note that I reduced the number of timers to 5K from 500K. Worth running on 500K timers too, perhaps.) |
|
ping @mscdex |
|
But you're now replacing public (linkedlist) API with private property manipulation. Do we wanna go here or are there other ways to optimize? Also, do we need this optimized further? (what are the real-world benefits?) |
The public API is deprecated. I don't think it's a big deal to not use it. As for real-world use cases, that's an excellent question that I hope someone else can shed light on. The benchmark we've had for years tests 500K timers, which seems like more than I would ever expect to really see, but time and again reality shows my imagination to be inadequate, so I don't know. I guess now is a good time to /cc @Fishrock123 |
|
@Trott I only meant to say: public to internals. We don't enjoy internals messing with an EventEmitter's internals either for the same reason, right? We know we can control it, but it's a bit hackish. I'll leave it there though :) Hoping to get some insight on the other questions. |
|
Bumping the benchmark to 10K: |
|
I've seen instances of similar things happening (especially so with V8 5.4 now) because of how the functions are compiled (TurboFan vs. Crankshaft), if they're inlined, etc. |
|
The "breadth" benchmark tests it with passing lots of arguments, right? I'm not really sure how this could effect that or how important that is. Also, there are two changes here: removing unnecessary asserts, and then moving the list logic. Additionally I would feel a lot better if we didn't rely on just the benchmark output here but also took a look at V8 performance profiles from the benchmarks by using |
We can not use it but I think we are probably stuck replicating it... |
|
Effect is smaller but still statistically significant with the default 500K timers. I wonder if we should reduce that benchmark to default to, say, 5K and 10K. It seems like 500K is more of a pummel test (will this run successfully after 20 minutes or crash?) and I wonder if the performance when the data is that large is more about the hardware and less about what Node.js does, so things that are hugely positive get watered down. |
|
I'd say try 100k -- anywhere below probably doesn't even matter for this fine grain of perf. (if you're trying that on a light laptop.. maybe we should try on a machine with better perf...) |
By moving the linked list manipulations out of a separate module and into `lib/timers.js`, a significant performance increase is seen in the timer benchamrks. I am seeing consistent 50% improvement with a p-value of .0005 for the "breadth" benchmark with "thousands" set to 5. This is probably due to removing the overhead of calling an external function and possibly also removing some special handling that is not needed in some cases.
|
Oh, wait, the breadth one is fast, it's the depth one that takes forever. So I can do 500K and just run the breadth test no problem. (I guess that's kind of what @ronkorving may have been alluding to indirectly--that this is already fast, does it really need to be faster in any real-world use cases? I don't know the answer to that.) So, here's 500K running the breadth test only: And the raw CSV generated: |
|
I should note that the two 500K benchmarks were run on different laptops. That may explain why one saw a larger benefit than the other. Regardless, both results have very low p-values (which is what you want--I'm sure someone will correct me on this because I'm being imprecise, but low p-value more or less is taken to equate to statistically significance). |
|
All TCP connections use timers. The way I last checked on the significance on impact in context was to make an http server with |
|
Closed? |
|
@ronkorving Yeah, I wasn't going to get around to benchmarking it any time soon, and I'm not sure if it will show a statistically significant benefit or not. I'm happy to re-open it if someone else wants to take on benchmarking it (per @Fishrock123's comment) and/or I'm happy to have someone enthusiastic about it take it on in their own pull request. |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
timers
Description of change
By moving the linked list manipulations out of a separate module and
into
lib/timers.js, a significant performance increase is seen in thetimer benchamrks. I am seeing consistent 50% improvement with a p-value
of .0005 for the "breadth" benchmark with "thousands" set to 5.
This is probably due to removing the overhead of calling an external
function and possibly also removing some special handling that is not
needed in some cases.