nsqd: replace util.UniqRands

[vearne]

The performance of util.UniqRands is not good, I re-implemented

[vearne]

action	Previous implementation (ns/op)	my implementation (ns/op)
select 20 from 100	2793	788
select 50 from 100	7812	1729
select 20 from 1000	2621	2152
select 50 from 1000	6764	3030

[ploxiln]

This switches the order of the arguments, but the caller still uses the original argument order.

I see you don't like l and n and that's fair, they're not super clear. But I find "count" being in both of your argument names to be confusing. I think we can come up with better names. Here's a proposal (back in the original order): (quantity, maxval int)

[vearne]

I accept your opinion, and I will fix it.

[ploxiln]

Since the default QueueScanSelectionCount is 20, I think the interesting benchmark cases are:

• select 20 from 5
• select 20 from 20
• select 20 from 50

And you could put the little benchmark function in internal/util/util_test.go (see for example nsqd/guid_test.go)

[ploxiln]

By the way, you can push updates to your branch, and the updates will show up in this pull request, so you don't have to close it and open a new one.

[vearne]

@ploxiln please review

[ploxiln]

The rand seed is set in apps/nsqd/nsqd.go, so we should not do that here.

I think we don't need to set a "good" random seed for tests (and they will be somewhat more deterministic if we don't).

[ploxiln]

I don't think it's worth both a comment and a function for this.
I would just put the line of code that does the swap here.

[ploxiln]

Thanks for the updates.

@mreiferson I think this change is worth it because the old implementation could have occasionally (randomly) bad worst-case performance - it could get "unlucky" and not randomly pick the one remaining possibility for a while. So, where it is used, the less-than-optimal average performance is not a problem at all. But the on-rare-occasion-terrible performance is worrysome. This new algorithm will always take a deterministic bounded number of steps (and the average performance is also better).

I had to tweak the benchmark to make it work with the old implementation, because the old implementation did not internally check for l > n:

+++ b/internal/util/util_test.go
@@ -6,7 +6,7 @@ import (
 func BenchmarkUniqRands20_5(b *testing.B) {
        for i := 0; i < b.N; i++ { //use b.N for looping
-               UniqRands(20, 5)
+               UniqRands(5, 5)
        }
@@ -30,7 +30,7 @@ func TestUniqRands(t *testing.T) {
                t.Errorf("error, expect:%v, got:%v\n", 3, len(x))
        }
 
-       x = UniqRands(10, 5)
+       x = UniqRands(5, 5)
        if len(x) == 5 {
                t.Logf("success, %v", x)

And the results are similar to those posted above. Before:

BenchmarkUniqRands20_5-4    	 2000000	       942 ns/op
BenchmarkUniqRands20_20-4   	  200000	      6889 ns/op
BenchmarkUniqRands20_50-4   	  500000	      3413 ns/op

After:

BenchmarkUniqRands20_5-4    	 5000000	       237 ns/op
BenchmarkUniqRands20_20-4   	 2000000	       835 ns/op
BenchmarkUniqRands20_50-4   	 2000000	       892 ns/op

[mreiferson]

LGTM, I’ll leave it for plo to merge when ready.

[ploxiln]

I screwed this up, sorry - I was trying to push one more commit to vearne's branch, but:

• pushed my master branch to his master branch
  • which I was allowed to do because his master branch was the branch used for this open PR in this project of which I am an admin
• pushing master made his master no longer contain any different commits, and so closed this PR
• this PR closed, I can no longer push to his (master) branch

so I'll have to do it in another PR, incoming

[ploxiln]

replaced with #1019