Skip to content

perf(node-http-handler): replace per-request checkSocketUsage timer with shared interval#1878

Closed
TrevorBurnham wants to merge 0 commit intosmithy-lang:mainfrom
TrevorBurnham:main
Closed

perf(node-http-handler): replace per-request checkSocketUsage timer with shared interval#1878
TrevorBurnham wants to merge 0 commit intosmithy-lang:mainfrom
TrevorBurnham:main

Conversation

@TrevorBurnham
Copy link
Contributor

Problem

Every call to NodeHttpHandler.handle() creates a setTimeout that invokes checkSocketUsage, and clears it when the request resolves or rejects. Under high concurrency, this means thousands of timer allocations and cleanup operations per batch, all for a diagnostic warning that only needs to fire periodically.

Solution

Replace the per-request setTimeout/clearTimeout pair with a single shared setInterval on the NodeHttpHandler instance, lazily created on the first handle() call via ensureSocketWarningInterval().

The interval checks both the HTTP and HTTPS agents on each tick. The existing 15-second lockout inside checkSocketUsage prevents log spam, same as before.

When both agents have maxSockets === Infinity, no interval is created at all; checkSocketUsage would bail out immediately anyway.

What changed

node-http-handler.ts:

  • Added socketWarningInterval field (tri-state: undefined → not yet initialized, null → not needed, Timeout → active).
  • Added ensureSocketWarningInterval() private method.
  • Removed the per-request timing.setTimeout + clearTimeout from handle().
  • destroy() clears the interval.
  • updateHttpClientConfig() resets the interval so it re-initializes with the updated config on the next handle() call.
  • Updated a stale comment inside checkSocketUsage that referenced the old per-request setTimeout cancellation behavior.

node-http-handler.spec.ts:

  • Updated the socketAcquisitionWarningTimeout test to spy on setInterval instead of timing.setTimeout.
  • Added a missing vi.spyOn(timing, "setTimeout") to the per-request requestTimeout test (it was implicitly relying on spy state leaked from the previous test).
  • Added three new tests:
    • No interval created when maxSockets is Infinity
    • Only one interval across multiple handle() calls
    • destroy() clears the interval

What did NOT change

  • checkSocketUsage static method — identical logic, just called from the interval instead of per-request timers.
  • All other timers in handle() (connection, request, socket, keepalive) — these are per-request by design and are untouched.
  • The warning message text and 15-second lockout behavior.
  • The socketAcquisitionWarningTimeout config option — still controls the interval delay, same semantics.

Benchmark results

Timer creation overhead measured across concurrency levels (5 iterations averaged):

Concurrency Old (per-request setTimeout) New (shared setInterval) Speedup
10 0.044 ms 0.004 ms ~10x
100 0.048 ms 0.003 ms ~16x
1,000 0.645 ms 0.020 ms ~32x
10,000 1.891 ms 0.080 ms ~24x

This measures timer creation overhead only. The old approach also incurred per-request clearTimeout cleanup cost on every resolve/reject path.

Benchmarking steps

Save the following as benchmark.ts anywhere in the repo and run it with
npx tsx benchmark.ts:

const CONCURRENCY_LEVELS = [10, 100, 1_000, 10_000];
const ITERATIONS = 5;

function benchmarkPerRequestTimers(concurrency: number) {
  const timeouts: ReturnType<typeof setTimeout>[] = [];
  const start = performance.now();
  for (let i = 0; i < concurrency; i++) {
    timeouts.push(setTimeout(() => {}, 3000));
  }
  const ms = performance.now() - start;
  for (const t of timeouts) clearTimeout(t);
  return ms;
}

function benchmarkSharedInterval(concurrency: number) {
  let interval: ReturnType<typeof setInterval> | null = null;
  const start = performance.now();
  for (let i = 0; i < concurrency; i++) {
    if (interval === null) {
      interval = setInterval(() => {}, 3000);
    }
  }
  const ms = performance.now() - start;
  if (interval) clearInterval(interval);
  return ms;
}

console.log("Concurrency".padEnd(14), "Old (per-req)".padEnd(20), "New (shared)".padEnd(20), "Speedup");
console.log("-".repeat(68));

for (const c of CONCURRENCY_LEVELS) {
  let oldTotal = 0, newTotal = 0;
  for (let i = 0; i < ITERATIONS; i++) {
    oldTotal += benchmarkPerRequestTimers(c);
    newTotal += benchmarkSharedInterval(c);
  }
  const oldAvg = oldTotal / ITERATIONS;
  const newAvg = newTotal / ITERATIONS;
  console.log(
    String(c).padEnd(14),
    `${oldAvg.toFixed(3)} ms`.padEnd(20),
    `${newAvg.toFixed(3)} ms`.padEnd(20),
    `${(oldAvg / newAvg).toFixed(1)}x`
  );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant