search

uber-archive / node-statsd-client

Node.js client for statsd
ISC License
28 stars 9 forks source link

Make sure that getChildClient() is cheap #33

Closed Raynos closed 8 years ago

Raynos commented 8 years ago

This makes two changes: first fix any missing options not passed to child.

Secondly ensure that all children share the same dnsResolver.

r: @Matt-Esch @syyang

Matt-Esch commented 8 years ago

You can make it cheaper if create a new constructor function with parent as proto, and override this.prefix

Raynos commented 8 years ago

Changing proto messes with the hidden class.

I can optimize the constructor further to make it cheaper but that seems like a micro-op. I just want to fix the shared DNSResolver lulz.

syyang commented 8 years ago

lgtm. thanks!

Raynos commented 8 years ago

before:

raynos at raynos-ThinkPad-T440p  ~/uber/node-statsd-client on fast-get-child-client*
$ node benchmarks/child-client.js 
time:  989
raynos at raynos-ThinkPad-T440p  ~/uber/node-statsd-client on fast-get-child-client*
$ node benchmarks/child-client.js 
time:  991
raynos at raynos-ThinkPad-T440p  ~/uber/node-statsd-client on fast-get-child-client*
$ node benchmarks/child-client.js 
time:  959

image

after:

$ node benchmarks/child-client.js 
time:  648
raynos at raynos-ThinkPad-T440p  ~/uber/node-statsd-client on fast-get-child-client*
$ node benchmarks/child-client.js 
time:  657
raynos at raynos-ThinkPad-T440p  ~/uber/node-statsd-client on fast-get-child-client*
$ node benchmarks/child-client.js 
time:  627

image

I made it 66% faster with the latest commit. Looking at the flames I think I made v8 inline all the constructors and stuff.

If it needs to be any faster i'm going to have to read the generated ASM: