freelist: Performance, replace shift() with pop() #2174
Conversation
|
Normally when we talk about performance, we create a benchmark suite to demonstrate the gain. Read more about it here https://github.com/nodejs/io.js/tree/master/benchmark |
|
I agree with @thefourtheye, it would also be good to see numbers from said benchmark, especially on the master and next branches (and maybe even next+1 if it's working yet). |
|
ref: #569 |
|
It will be difficult to test this directly since it's an internal, and a micro benchmark between the two operations would be bogus. How could it be exposed for testing? |
|
@trevnorris We can use |
|
@thefourtheye Ah yes. Completely forgot about that one. Thanks for the reminder. |
|
Thanks for your patience. Here's a benchmark commit (I hope, I did it right). |
|
@subzey If you can post the code in a gist I can help you write up a benchmark if you need it. |
|
@trevnorris Does 3531e82 look good? |
|
@subzey Other than some style nits (spacing 'n stuff) it looks good. |
|
if it's an issue can be easily enough taken care of before we land it. just keep the reference for future contributions. ;) |
|
@subzey Which branch did you test on to get those numbers? |
|
@mscdex, Latest release. |
|
Looks like this microoptimization is useless now (#2176), feel free to reject this PR. |
|
@subzey Don't worry about it. Its deprecated for the users. It is still used internally by http module. |
|
@brendanashworth I'm sorry, should I just merge |
|
@subzey He's asking if you could rebase against |
Fishrock123
added
the
benchmark
|
Force-pushed a rebased commit into this PR; fixed eslint whitespace errors |
|
@subzey Can you share some performance numbers, with |
|
@thefourtheye, Sure. |
|
@subzey You might want to change the corresponding test as well, |
|
@thefourtheye, you're right. I'm terribly sorry for breaking the test, I'll fix it asap |
|
|
||
|
|
||
| // First, alloc N items | ||
| for (i = 0; i < n; i++) { |
There was a problem hiding this comment.
This benchmark doesn't really handle the common case (and exits way too fast). Do you think you could change it so it allocates / frees / allocates in blocks of say 500, all the way up to n? We try to have benchmarks take about 5-10 seconds to execute so numbers don't jump around as much.
|
@subzey ... is this still something you'd like to pursue? |
|
Is there any info I can provide? |
| 'use strict'; | ||
|
|
||
| var common = require('../common.js'); | ||
| var FreeList = require('../../lib/internal/freelist').FreeList; |
There was a problem hiding this comment.
This should be require('internal/freelist') with a note at the top that --expose-internals is needed to run the benchmark. Until benchmarks get special "Flags" code comments that get parsed like tests have, flags will need to be explicitly specified on the command-line.
|
A few nits but otherwise LGTM |
| for (i = 0; i < n; i++){ | ||
| // Return all the N to the pool | ||
| for (j = 0; j < poolSize; j++) { | ||
| list.free(used[i]); |
|
@ofrobots, @mscdex. The nits were fixed, I've squashed all the changes into 1 commit and rebased it on top of master (as I was asked previously). And also tried to make a perfect commit message. Please take a look.
A...actually, I'd prefer to do it in a separate branch. :) |
| bench.start(); | ||
|
|
||
| for (i = 0; i < n; i++){ | ||
| // Return all the N to the pool |
Array#pop() is known to be faster than Array#shift(). To be exact, it's O(1) vs. O(n). In this case there's no difference from which side of the "pool" array the object is retrieved, so .pop() should be preferred.
Fixed |
jasnell
added
lts-watch-v4.x
and removed
stalled
ChALkeR
added
the
performance
|
LGTM |
|
LGTM. Landed as c647e87. |
Array#pop() is known to be faster than Array#shift(). To be exact, it's O(1) vs. O(n). In this case there's no difference from which side of the "pool" array the object is retrieved, so .pop() should be preferred. PR-URL: #2174 Reviewed-By: mscdex - Brian White <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]> Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
Array#pop() is known to be faster than Array#shift(). To be exact, it's O(1) vs. O(n). In this case there's no difference from which side of the "pool" array the object is retrieved, so .pop() should be preferred. PR-URL: #2174 Reviewed-By: mscdex - Brian White <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]> Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
Array#pop() is known to be faster than Array#shift(). To be exact, it's O(1) vs. O(n). In this case there's no difference from which side of the "pool" array the object is retrieved, so .pop() should be preferred. PR-URL: #2174 Reviewed-By: mscdex - Brian White <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]> Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
Array#pop() is known to be faster than Array#shift(). To be exact, it's O(1) vs. O(n). In this case there's no difference from which side of the "pool" array the object is retrieved, so .pop() should be preferred. PR-URL: #2174 Reviewed-By: mscdex - Brian White <[email protected]> Reviewed-By: jasnell - James M Snell <[email protected]> Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
Array#pop() is known to be faster than Array#shift().
In this case there's no difference from which side of the "pool" array
the object is retrieved.