Goroutine Pool for UDP datagram reads

[horahoradev]

This PR puts UDP server datagram reads into a Goroutine pool, and was made to address this issue within CoreDNS: coredns/coredns#5595 (comment)

Here are some benchmarks of CoreDNS with and without my changes, with one server, using DNSperf.

Without my change:

  Queries sent:         13988687
  Queries completed:    13988288 (100.00%)
  Queries lost:         300 (0.00%)
  Queries interrupted:  99 (0.00%)

  Response codes:       NOERROR 13988288 (100.00%)
  Average packet size:  request 32, response 100
  Run time (s):         105.433637
  Queries per second:   132673.863845

  Average Latency (s):  0.000627 (min 0.000023, max 0.022370)
  Latency StdDev (s):   0.000151

CPU utilization ~420%

With my change:

  Queries sent:         5735429
  Queries completed:    5735336 (100.00%)
  Queries lost:         0 (0.00%)
  Queries interrupted:  93 (0.00%)

  Response codes:       NOERROR 5735336 (100.00%)
  Average packet size:  request 32, response 100
  Run time (s):         29.624372
  Queries per second:   193601.943697

  Average Latency (s):  0.000483 (min 0.000014, max 0.009152)
  Latency StdDev (s):   0.000336

CPU utilization ~560%

So notably we see CPU utilization went up significantly but QPS went up ~50%, and avg latency is down.

This is a pretty stupid change, but it seems pretty impactful. Let me know if there's anything you'd like to see here; from my interpretation, all affected method calls should be thread-safe (but let me know if I've missed anything!).

[tmthrgd]

I’d like to see this with errgroup as I think that’s going to be better here.

[horahoradev]

Thanks for the quick review! I changed the waitgroup/error channel out for an error group.
I opted not to cause the rest of the Goroutine read loops to terminate if one of them returns an error, but let me know if you'd like to see something different here.

[miekg]

@tmthrgd is this good to merge?

[tmthrgd]

@miekg I'm not a huge fan. Pools and multiple readers are hard to get right in a general sense. What might improve things for one workload, could easily worsen things in others. In particular I know things can get very counterintuitive with high core count CPUs.

[tmthrgd]

This isn't formatted properly. Stdlib should be grouped up separately from other imports.

[tmthrgd]

There's nothing to actually ensure this call unblocks when the errgroup returns with an error. This could easily end up still reading in one go routine while others have already exited.

[tmthrgd]

var g errgroup.Group

[tmthrgd]

I don't know if NumCPU is correct here or not.

[tmthrgd]

Maybe we could define some sort of interface for people who want to implement pools of some kind. I'm not sure.

[miekg]

Ack. Thanks for your thoughts. Tend to agree, so I'll close this PR. As for an interface, nothing forces you to use all the things in this library, so prolly best to not define anything for this use case

…

On Sat, 29 Apr 2023, 06:46 Tom Thorogood, ***@***.***> wrote: Maybe we could define some sort of interface for people who want to implement pools of some kind. I'm not sure. — Reply to this email directly, view it on GitHub <#1416 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWIW6KYTAZCOW4CI6UK5LXDSMK3ANCNFSM6AAAAAAURK3OQ4> . You are receiving this because you were mentioned.Message ID: ***@***.***>