Skip to content
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

nsqlookupd: use map for producers - with benchmarks #1038

Merged
merged 2 commits into from
Jun 3, 2018

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Jun 2, 2018

I refactored and added a bit to @andyxning's benchmark from #1036

Results:

benchmark                                  old ns/op     new ns/op     delta
BenchmarkLookupRegistrations8x8-4          1065          1088          +2.16%
BenchmarkLookupRegistrations8x64-4         2264          1165          -48.54%
BenchmarkLookupRegistrations64x64-4        18669         8364          -55.20%
BenchmarkLookupRegistrations64x512-4       134794        10933         -91.89%
BenchmarkLookupRegistrations512x512-4      1698650       150939        -91.11%
BenchmarkLookupRegistrations512x2048-4     5751659       177923        -96.91%
BenchmarkFindProducers8x8-4                244           619           +153.69%
BenchmarkFindProducers8x64-4               243           2259          +829.63%
BenchmarkFindProducers64x64-4              244           2437          +898.77%
BenchmarkFindProducers64x512-4             245           16447         +6613.06%
BenchmarkFindProducers512x512-4            277           16825         +5974.01%
BenchmarkFindProducers512x2048-4           277           83689         +30112.64%

take a look @mreiferson

@ploxiln
Copy link
Member Author

ploxiln commented Jun 2, 2018

After that, I realized: we usually look-up topics, not channels, which my benchmark was looking up producers for. And it looks like there's some "filtering" code that kicks in sometimes. I should probably benchmark looking up producers of topics.

So I split out topic-lookup benchmarks. The results were pretty much the same as for channel lookup.

But I realized that the typical /lookup operation of nsqlookups involved FindRegistrations(), and other stuff, so now I figure I should make a benchmark that more closely matches what doLookup() actually does ...

@ploxiln ploxiln force-pushed the lookupd_producer_map branch from 09e0aaa to 0dbe591 Compare June 2, 2018 19:20
@ploxiln
Copy link
Member Author

ploxiln commented Jun 2, 2018

and the results:

$ benchcmp old.txt new.txt 
benchmark                                  old ns/op     new ns/op     delta
BenchmarkLookupRegistrations8x8-4          2969          2923          -1.55%
BenchmarkLookupRegistrations8x64-4         6334          3298          -47.93%
BenchmarkLookupRegistrations64x64-4        55507         30026         -45.91%
BenchmarkLookupRegistrations64x512-4       396369        39583         -90.01%
BenchmarkLookupRegistrations512x512-4      5135966       515863        -89.96%
BenchmarkLookupRegistrations512x2048-4     16354360      595718        -96.36%
BenchmarkDoLookup8x8-4                     1477          1885          +27.62%
BenchmarkDoLookup8x64-4                    1462          3584          +145.14%
BenchmarkDoLookup64x64-4                   6189          8770          +41.70%
BenchmarkDoLookup64x512-4                  6387          23065         +261.12%
BenchmarkDoLookup512x512-4                 45920         65297         +42.20%
BenchmarkDoLookup512x2048-4                45398         119196        +162.56%

It's probably a good idea to add benchmarks for typical lookup-protocol IDENTIFY and REGISTER operations (mostly AddProducer()). LookupRegistrations() is only called when a producer disconnects a tcp-lookup-protocol connection. (But it can still be pretty bad for it to lock the RegistrationDB for a long time.)

@ploxiln
Copy link
Member Author

ploxiln commented Jun 2, 2018

Results for the new REGISTER bench:

benchmark                                  old ns/op       new ns/op      delta
BenchmarkRegister8x8-4                     80615           79249          -1.69%
BenchmarkRegister8x64-4                    733022          741142         +1.11%
BenchmarkRegister64x64-4                   6035122         6548519        +8.51%
BenchmarkRegister64x512-4                  179271032       52564415       -70.68%
BenchmarkRegister512x512-4                 1505092668      416921240      -72.30%
BenchmarkRegister512x2048-4                17299945707     1770267744     -89.77%

@andyxning
Copy link
Member

@ploxiln Thanks for the more detailed benchmark. The new scheme's decreased performance may be due to the map allocation and adjustment.

But the lookup is called periodically and the register is only called on the start of nsqd and new consumer connections.

@ploxiln
Copy link
Member Author

ploxiln commented Jun 3, 2018

3x slower lookup may be an OK tradeoff for 10x faster register and 30x faster disconnect. nsqd do disconnect and reconnect now and then, especially in large clusters. Absolute numbers are also good to consider: the new slower topic-lookup is 100us, while the old slower disconnect-related registrations-lookup is 16000us. (The Register benchmark is a bit harder to interpret, a single iteration includes a whole set of producers. So for the old slower value for 512 topics and 2048 producers, it's 12 seconds / 2048 producers = 6000us per producer doing REGISTER.)

I've re-run the benchmarks on a desktop computer with a good cpu cooler, which gives more consistent results than my laptop:

benchmark                                  old ns/op       new ns/op      delta
BenchmarkLookupRegistrations8x8-4          3014            3004           -0.33%
BenchmarkLookupRegistrations8x64-4         6195            3701           -40.26%
BenchmarkLookupRegistrations64x64-4        57157           30230          -47.11%
BenchmarkLookupRegistrations64x512-4       264357          28423          -89.25%
BenchmarkLookupRegistrations512x512-4      4043190         412344         -89.80%
BenchmarkLookupRegistrations512x2048-4     16327432        461220         -97.18%
BenchmarkRegister8x8-4                     68448           69000          +0.81%
BenchmarkRegister8x64-4                    631129          697277         +10.48%
BenchmarkRegister64x64-4                   5061038         7033682        +38.98%
BenchmarkRegister64x512-4                  135048559       43975672       -67.44%
BenchmarkRegister512x512-4                 1087140586      344428911      -68.32%
BenchmarkRegister512x2048-4                12703684561     1461857628     -88.49%
BenchmarkDoLookup8x8-4                     1286            1597           +24.18%
BenchmarkDoLookup8x64-4                    1255            3393           +170.36%
BenchmarkDoLookup64x64-4                   5336            7541           +41.32%
BenchmarkDoLookup64x512-4                  5470            18457          +237.42%
BenchmarkDoLookup512x512-4                 38893           53696          +38.06%
BenchmarkDoLookup512x2048-4                39064           108361         +177.39%

@mreiferson
Copy link
Member

I think I agree, since lookups can also happen in parallel.

Thanks for this — do you want to clean up these commits before we merge?

Copy link
Member

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

andyxning and others added 2 commits June 3, 2018 13:43
with typical REGISTER ops, typical doLookup() ops,
and LookupRegistrations() which is part of disconnect

thanks Andy Xie for the first version of the LookupRegistration() bench
@ploxiln ploxiln force-pushed the lookupd_producer_map branch from 8c9e8b7 to f8b49e6 Compare June 3, 2018 17:46
@ploxiln
Copy link
Member Author

ploxiln commented Jun 3, 2018

squashed

@mreiferson mreiferson merged commit 3bd8b9d into nsqio:master Jun 3, 2018
@ploxiln ploxiln deleted the lookupd_producer_map branch June 3, 2018 20:30
@andyxning
Copy link
Member

@ploxiln @mreiferson Thanks for reviewing and merging this PR. 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants