perf(ext/node): bypass dns lookup on literal IPs in udp send - #35909
Conversation
Deno Individual Contributor License AgreementAll contributors have signed the CLA. Thank you! This is an automated message from CLA Assistant |
|
This looks like a random drive-by change. Can you show benchmarks proving this helps? |
|
@bartlomieju This definitely isn't a random drive-by change; We noticed that the IP address of the EC2 instance we were running our local Datadog agent on had 100x more Currently, when sending UDP packets to a literal IP (like By bypassing this, we get a direct path to the OS socket on the next tick, stop the trace spam, and free up the thread-pool." Here is a script to benchmark the throughput of 10,000 unconnected UDP sends to a literal IP including its benchmark results: bench_udp_send.jsimport dgram from "node:dgram";
import { Buffer } from "node:buffer";
const server = dgram.createSocket("udp4");
server.on("message", () => {});
await new Promise((resolve) => server.bind(0, "127.0.0.1", resolve));
const port = server.address().port;
const msg = Buffer.from("hello world");
Deno.bench({
name: "unconnected udp send to literal IP",
async fn() {
const client = dgram.createSocket("udp4");
// Blast 10,000 packets
const ITERS = 10000;
let sent = 0;
await new Promise((resolve, reject) => {
function sendNext() {
if (sent >= ITERS) {
client.close();
return resolve();
}
client.send(msg, 0, msg.length, port, "127.0.0.1", (err) => {
if (err) return reject(err);
sent++;
sendNext(); // chained sends to measure event loop churn
});
}
sendNext();
});
}
});Benchmark ResultsRunning the above script locally (Intel Core i9-14900HX, Windows x64) via Baseline (
Optimized (
ConclusionBy skipping the unnecessary |
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks for the PR, and especially for the detailed writeup and the AI disclosure — the context made it easy to trace this back to the upstream change it's porting (nodejs/node#64133, commit 1c12dd63479 "dgram: skip dns.lookup() for literal IP addresses").
I dug into it against that Node commit. There are a couple of ways the current approach diverges from Node, plus a Deno-specific snag that makes the "skip" trickier than it looks. Rather than push changes onto your branch, I wanted to lay out what I found and ask whether you'd be up for reworking it.
1. Node does the bypass inside the lookup function, not in send(). Upstream only touches lib/internal/dgram.js: it adds defaultLookup(address, family, callback) and installs it as the default lookup in newHandle(). Because the check lives in the lookup function, it covers bind(), connect() and send() uniformly. This PR only patches the unconnected send() path, so bind()/connect() to a literal still go through the full dns.lookup().
2. The check should be family-aware. Node uses isIP(address) === family, whereas this PR uses just isIP(address) (truthy for both 4 and 6). So a udp6 socket sending to an IPv4 literal (or a udp4 socket to an IPv6 literal) would bypass and hand the wrong-family address straight through, where Node deliberately falls back to dns.lookup(). Node added a test for exactly this — test-dgram-default-lookup-ip.js, the "mismatched family falls through" case.
3. Heads-up on a Deno-specific snag — this is the main reason I'm suggesting a rework rather than just "move it to internal/dgram.ts". Deno's own dns.lookup() already short-circuits literal IPs via process.nextTick(), and it only works because the default lookup path loads node:dns, which bootstraps the node process and enables nextTick. If defaultLookup bypasses dns.lookup entirely, nextTick() can be silently dropped when a socket bound/sent to a literal IP is the program's first async work (process not yet bootstrapped) — which breaks bind() completion and message reception. I confirmed this locally: socket.bind(0, "127.0.0.1", cb) as the first thing a program does never fires cb, and a node:dgram-only unit test like "udp ref and unref" starts failing. Importing node:process first (or otherwise touching node:dns) makes it work again. So a faithful port needs to keep the process bootstrapped — e.g. still touch node:dns when installing the default lookup — or defer in a way that doesn't depend on nextTick being enabled.
Would you be open to reworking it along Node's structure — a family-aware defaultLookup in internal/dgram.ts, with the bootstrap point in mind — and porting the three upstream test changes (test-dgram-default-lookup-ip.js new, plus the updates to test-dgram-custom-lookup.js and test-dgram-implicit-bind-failure.js)? Happy to help if you hit the nextTick/bootstrap issue.
One last note so you can calibrate: because Deno's dns.lookup() already avoids real resolution for literals, the perf win here is smaller than in Node (mostly avoiding the dns.lookup call and its diagnostics_channel/async_hooks overhead). The family-correctness part is still a nice improvement, so it's worth doing — just no rush.
|
I'm absolutely open to reworking this to align with Node's structure. I'll move the bypass into a family-aware I'll also port over the relevant upstream tests ( Give me a bit of time to put together the new approach and I'll update the PR. Thanks again for the guidance! |
Yeah, I will sync these up in the next two weeks - for now porting them to tests/unit_node/ will be more than enough, thanks! |
7a8e2c7 to
9768353
Compare
|
@bartlomieju Alright, the rework is pushed! I've faithfully mirrored Node's architecture by moving the bypass into a family-aware To handle the Deno-specific snag you mentioned, I added an explicit Could you please approve the CI workflow to run the tests and take another look when you have a moment? |
|
I'll upgrade our Node compat test suite to the latest upstream version (which picks up the updated test) and rebase this PR on top of it. After that the node_compat job should pass. |
Enable parallel/test-dgram-default-lookup-ip.js now that the node_compat suite has been bumped, and remove the tests/unit_node/dgram_test.ts ports that stood in for the upstream tests. Also load node:process instead of node:dns before scheduling the nextTick: nextTick() silently drops callbacks until node:process has been bootstrapped, which made bind() never complete.
Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
Ports the
defaultLookupoptimization from Node'slib/internal/dgram.js.When the address handed to
bind()orsend()is already a literal IP ofthe socket's own family, we resolve it to itself on the next tick instead of
routing it through
dns.lookup().Previously
newHandle()eagerly capturedlazyDns().default.lookupas thedefault lookup function, so creating any unconnected socket pulled in
node:dns(and the cares binding with it) even when every destination was aliteral IP. Deno's
dns.lookup()already short-circuits literal IPs throughnextTick(), so this does not change how many ticks a send takes — what itsaves is that module load plus the per-call option parsing and validation
inside
lookup().Addresses that are not a literal IP of the socket's family still go through
dns.lookup(), so a hostname resolves normally and'::1'on audp4socket keeps falling through to the resolver rather than being
short-circuited. A user-supplied
lookupoption is untouched:defaultLookupis only installed when no lookup was provided. Because
dns.lookupis nowread at call time rather than captured at socket construction, monkey-patching
it after
createSocket()takes effect the way it does in Node.nextTick()silently drops callbacks untilnode:processhas beenbootstrapped, so the fast path loads it first, the same way
ext/node/polyfills/01_require.jsdoes.Enables the upstream
parallel/test-dgram-default-lookup-ip.js, which coversthe IPv4 send fast path, the IPv6 bind fast path, and the mismatched-family
fallback. The existing
parallel/test-dgram-custom-lookup.jscontinues tocover custom lookups and hostname forwarding.
AI Disclosure:
This PR was written with the assistance of an AI coding assistant (Gemini 3.1
Pro / Antigravity).