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.

Sign up for GitHub

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

Jump to bottom

nsqlookupd: enhance performance of /nodes #1099

Merged
merged 1 commit into from
Nov 11, 2018
Merged

nsqlookupd: enhance performance of /nodes #1099

merged 1 commit into from
Nov 11, 2018

Conversation

andyxning
Copy link
Member

@andyxning andyxning commented Nov 8, 2018 •
edited
Loading

Enhance performance for nodes api.

This PR will avoid an additional loop to produce the final result.

The cpu pprof shows that the map iterator of ProducerMap2Slice will consume most of the CPU cycle when requesting nodes api.

go tool pprof 'http://10.8.50.84:4161/debug/pprof/profile'
Fetching profile over HTTP from http://10.8.50.84:4161/debug/pprof/profile
Local symbolization failed for nsqlookupd-0.3.8-ttv2: open /data00/tiger/nsq_deploy/bin/nsqlookupd-0.3.8-ttv2: no such file or directory
Some binary filenames not available. Symbolization may be incomplete.
Try setting PPROF_BINARY_PATH to the search path for local binaries.
Saved profile in /Users/andy/pprof/pprof.nsqlookupd-0.3.8-ttv2.samples.cpu.002.pb.gz
File: nsqlookupd-0.3.8-ttv2
Type: cpu
Time: Nov 8, 2018 at 11:35am (CST)
Duration: 30.09s, Total samples = 17.16s (57.02%)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top10
Showing nodes accounting for 10540ms, 61.42% of 17160ms total
Dropped 270 nodes (cum <= 85.80ms)
Showing top 10 nodes out of 129
flat flat% sum% cum cum%
5690ms 33.16% 33.16% 6450ms 37.59% runtime.mapiternext
860ms 5.01% 38.17% 11900ms 69.35% github.com/nsqio/nsq/nsqlookupd.(*httpServer).doNodes
770ms 4.49% 42.66% 9630ms 56.12% github.com/nsqio/nsq/nsqlookupd.ProducerMap2Slice
680ms 3.96% 46.62% 720ms 4.20% runtime.heapBitsSetType
560ms 3.26% 49.88% 560ms 3.26% runtime.memclrNoHeapPointers
560ms 3.26% 53.15% 1450ms 8.45% runtime.scanobject
380ms 2.21% 55.36% 430ms 2.51% runtime.mapaccess2_faststr
370ms 2.16% 57.52% 370ms 2.16% runtime.markBits.isMarked (inline)
360ms 2.10% 59.62% 630ms 3.67% encoding/json.(*encodeState).string
310ms 1.81% 61.42% 1960ms 11.42% runtime.gcDrain
(pprof)

@andyxning
Copy link
Member Author

@mreiferson @ploxiln

@andyxning andyxning force-pushed the lookupd_enhance_perf_for_nodes_api branch from 151e1c7 to 2e55bf2 Compare November 8, 2018 08:01
@andyxning andyxning changed the title lookupd: enhance performance for nodes api [WIP] lookupd: enhance performance for nodes api Nov 8, 2018
@ploxiln
Copy link
Member

ploxiln commented Nov 8, 2018

I did notice the first version, it was pretty simple ... the latest version fails tests with:

panic: assignment to entry in nil map

github.com/nsqio/nsq/nsqlookupd.(*RegistrationDB).AddProducer(0xc420122420, 0x8d7cae, 0x6, 0x8d5d70, 0x0, 0x8d5d70, 0x0, 0xc420123440, 0xc420134900)
	/home/travis/gopath/src/github.com/nsqio/nsq/nsqlookupd/registration_db.go:83 +0x543
github.com/nsqio/nsq/nsqlookupd.(*LookupProtocolV1).IDENTIFY(0xc42000e308, 0xc42012aea0, 0xc420021500, 0xc420121cd0, 0x0, 0x0, 0xc420121cd0, 0xc420134c28, 0x4e433b, 0xc420121cc0, ...)
	/home/travis/gopath/src/github.com/nsqio/nsq/nsqlookupd/lookup_protocol_v1.go:228 +0xb7b

@andyxning
Copy link
Member Author

@ploxiln Thanks for the review. I do not have test this locally and this is why i add WIP prefix to the title of the PR. I will fix this later today.

@andyxning andyxning force-pushed the lookupd_enhance_perf_for_nodes_api branch 5 times, most recently from eb5944b to fa8e39c Compare November 9, 2018 10:43
@andyxning andyxning changed the title [WIP] lookupd: enhance performance for nodes api lookupd: enhance performance for nodes api Nov 9, 2018
@andyxning
Copy link
Member Author

@ploxiln After apply the change. One of the clusters in our production reduce nodes api response time from 0m0.527s to 0m0.096s.

ploxiln
ploxiln reviewed Nov 10, 2018
View reviewed changes
nsqlookupd/http.go
topicProducersMap[t] = s.ctx.nsqlookupd.DB.FindProducers("topic", t, "")
}

topicProducers := topicProducersMap[t]
for _, tp := range topicProducers {
if tp.peerInfo == p.peerInfo {
tombstones[j] = tp.IsTombstoned(s.ctx.nsqlookupd.opts.TombstoneLifetime)
Copy link
Member

Choose a reason for hiding this comment

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

I think that adding a break just after this line is an easy way to make this slightly faster

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL.

@ploxiln
Copy link
Member

ploxiln commented Nov 10, 2018

Looks good.

It's funny that doNodes() gets a list of all producers, and for each it gets a list of all topics, and then for each topic it gets all producers, just to get the tombstoned state for the producer+topic it is working on ... maybe this could be refactored more, but I'm admittedly too rusty with this codebase to really analyze this at the moment.

Anyway, not invasive, looks like a good little optimization.

@andyxning andyxning force-pushed the lookupd_enhance_perf_for_nodes_api branch from fa8e39c to d84a2d8 Compare November 10, 2018 13:46
@ploxiln
Copy link
Member

ploxiln commented Nov 10, 2018

Still looks good to me :)
I'm just going to wait a couple of days to give the other maintainers a chance to have a look.

@andyxning
Copy link
Member Author

@mreiferson PTAL.

mreiferson
mreiferson approved these changes Nov 11, 2018
View reviewed changes
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, thanks!

@mreiferson mreiferson merged commit b0df52b into nsqio:master Nov 11, 2018
@mreiferson mreiferson changed the title lookupd: enhance performance for nodes api nsqlookupd: enhance performance for nodes api Nov 11, 2018
@mreiferson mreiferson changed the title nsqlookupd: enhance performance for nodes api nsqlookupd: enhance performance for /nodes Nov 11, 2018
@mreiferson mreiferson changed the title nsqlookupd: enhance performance for /nodes nsqlookupd: enhance performance of /nodes Nov 11, 2018
@andyxning andyxning deleted the lookupd_enhance_perf_for_nodes_api branch November 12, 2018 01:34
@andyxning
Copy link
Member Author

Thanks for you @mreiferson and @ploxiln

andyxning added a commit to andyxning/nsq that referenced this pull request Nov 5, 2019
@andyxning
backport: nsqio#1099
875829f
andyxning pushed a commit to andyxning/nsq that referenced this pull request Nov 5, 2019
Merge branch 'backport_1099' into 'ttv2_v0.3.8'
fb139c5 
backport: nsqio#1099

See merge request nsq/nsq!34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ploxiln ploxiln ploxiln left review comments

@mreiferson mreiferson mreiferson approved these changes

Assignees
No one assigned
Labels
perf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyxning @ploxiln @mreiferson