Skip to content

fix: avoid quadratic analysis time on large files - #613

Merged
styfle merged 2 commits into
vercel:mainfrom
tycrimm:fix/quadratic-static-eval
Nov 5, 2026
Merged

styfle merged 2 commits into
vercel:mainfrom
tycrimm:fix/quadratic-static-eval

Conversation

@tycrimm

@tycrimm tycrimm commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

computePureStaticValue builds a fresh vars object on every call and copies
every global and known top-level binding into it. It runs for most call
expressions and variable declarations, so analysis costs calls × bindings.
That grows with the square of the file size. Large bundled files, such as a
server build that inlines its dependencies, pay the most.

evaluate only reads vars by name. This PR passes a Proxy that looks each
name up in knownBindings and globalBindings when it is read, in the same
order as before: import.meta, then known bindings (shadowed ones still
resolve to undefined), then globals. knownBindings changes only in the
walker, and the walker awaits each call, so a lookup sees the same state as the
old copy.

Benchmark. A synthetic file with N top-level constants, each used once
(Node 26, script below):

Bindings main This PR
1,000 120ms 30ms
2,000 529ms 40ms
4,000 2,387ms 118ms
8,000 10,745ms 368ms

On a real 17 MB bundled server file, analysis dropped from 10.2s to 1.4s. On a
real app with 5,707 traced files, the traced file list was identical before
and after.

Benchmark script
// node bench.mjs <path-to-built-nft>
import { writeFileSync, mkdirSync } from 'node:fs';
import path from 'node:path';
const { nodeFileTrace } = await import(path.resolve(process.argv[2], 'out/index.js'));
const dir = path.resolve('synth-out');
mkdirSync(dir, { recursive: true });
for (const n of [1000, 2000, 4000, 8000]) {
  let src = '';
  for (let i = 0; i < n; i++) src += `const c${i} = "value-${i}";\nconst v${i} = String(c${i});\n`;
  const file = path.join(dir, `bindings-${n}.js`);
  writeFileSync(file, src);
  const t = performance.now();
  await nodeFileTrace([file], { base: dir });
  console.log(`${n} bindings: ${Math.round(performance.now() - t)}ms`);
}

Tests. jest: 1466/1470 pass, the same as on main. The 4 failures also
fail on main locally: leveldown and sharp-pnpm (native builds), and
asset-fs-extra from cwd and from root.

🤖 Generated with Claude Code

@tycrimm
tycrimm requested review from a team, icyJoseph, ijjk and styfle as code owners September 24, 2026 16:29
@BCarroll524

Copy link
Copy Markdown

We hit this in production builds and can confirm the fix works on a real app.

Setup: Remix 2.16 + Vite on Vercel. @vercel/remix-builder 27.0.1 traces the server bundle with nft 1.10.0. The main server bundle is a single 32 MB file, because the Remix server build imports every route and we inline a number of dependencies.

What we measured on that file:

  • 6,526 known top-level bindings and ~1.2M computePureStaticValue calls, so about 1.16 billion binding copies.
  • A 7 MB server bundle from the same app does about 125M copies and traces in ~13s.
  • Tracing the 32 MB file takes 60–112s locally. On Vercel it's about 5.5 minutes of a ~9 minute build, with no log output during that time.

With the same change as this PR (a Proxy that looks bindings up when read instead of copying them), applied to 1.10.0 locally:

  • Trace time dropped from 59.7s to 11.1s in back-to-back runs.
  • fileList was identical: 12,344 files, no differences.

We can't patch nft inside Vercel's build image, so we're waiting on a release. It would be great to get this reviewed and shipped, and picked up by the Remix and React Router builders. Happy to share more profiling details if useful.

@styfle styfle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: commits must be signed before this can merge — dcd1e60 is currently unsigned. Please re-sign and force-push.

Non-blocking: the proxy only implements getOwnPropertyDescriptor and get, which is all evaluate reaches for today. If static-eval later uses name in vars, Object.keys(vars), or assigns a binding, those will silently hit the empty target instead of the real bindings. A has trap and a throwing set would be cheap insurance against that.

Reviewed via styfle's devbox

computePureStaticValue copied every global and known top-level binding
into a new object on each call, so analysis cost calls x bindings. Look
bindings up on demand through a Proxy instead, in the same order. The
proxy also answers `in` checks and rejects writes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tycrimm
tycrimm force-pushed the fix/quadratic-static-eval branch from dcd1e60 to 1adf79f Compare November 3, 2026 23:11
@tycrimm

tycrimm commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @styfle. Re-signed and force-pushed as 1adf79f (verified). Also added the has trap and a throwing set on the proxy.

styfle commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

CI is red on all four ubuntu-latest jobs. Every test passes — the failure is the coverage gate:

Jest: "global" coverage threshold for functions (96.25%) not met: 95.32%

(ubuntu runs test-verbose, which adds --coverage; macOS/Windows run plain test, which is why those are green)

It's the functions metric, not branches — the has and set traps are never invoked, and they are the only two unhit functions in out/analyze.js. Per-function hits from a local --coverage run:

out/analyze.js hits
getVarDescriptor 1,826,477
getOwnPropertyDescriptor trap 1,603,693
get trap 222,784
has trap 0
set trap 0

Nothing reaches them today since static-eval uses Object.hasOwnProperty.call rather than in and never assigns. I'd still rather not drop them: without has, a future name in vars silently returns false for every binding, and without set, an assignment silently disappears into the empty target. Those are the silent-wrong-answer cases worth guarding.

So let's cover them instead. The proxy is stateless per analyze() call — it only reads knownBindings, globalBindings and importMetaUrl — so it can be lifted out of computePureStaticValue into an exported factory and tested directly; the tests already reach into out/ (require('../out/analyze.js'), require('../out/fs')). A handful of assertions on the lookup contract would cover has and set and pin the equivalence this PR rests on: precedence (import.meta → known → global), a shadowed binding reading as present-with-undefined, an undefined-valued global like MONGOOSE_DRIVER_PATH reading as present, and toString/constructor not leaking in from Object.prototype.

If you'd rather not restructure, bumping functions in jest.config.js would unblock and I think thats acceptable.

Non-blocking: lifting the proxy out of computePureStaticValue is worth doing on its own. The target, handler object and four closures are re-allocated on every call — ~1.8M calls just to get through this repo's test suite — and none of it depends on per-call state.

Reviewed via styfle's devbox

Build the proxy once per analyze() through an exported factory instead
of on every computePureStaticValue call, and test it directly so the has
and set traps are covered. The tests pin the old eager copy's behavior:
lookup order, shadowed bindings, undefined-valued globals, and no
Object.prototype leaks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tycrimm

tycrimm commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Went with covering them. e0d4f52 (signed) lifts the proxy into an exported createStaticEvalVars(knownBindings, globals, importMetaUrl), built once per analyze() call instead of once per evaluation, and adds test/static-eval-vars.test.js. The tests check, name by name, that the proxy matches the old eager copy (hasOwnProperty, in, and value). They also cover lookup order, a shadowed binding reading as present with undefined, an undefined-valued global, Object.prototype names and symbols not leaking in, live updates, and set throwing.

Function coverage locally is 96.51% (threshold 96.25%), with the same environment-only failures as before this commit. A mutation that drops the shadow check fails the tests.

@styfle styfle left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good 👍

Non-blocking: the shadow rule now exists in two places, getKnownBinding and getDescriptor's binding.shadowDepth === 0 ? binding.value : undefined. They agree today, but a change to one won't reach the other. Passing getKnownBinding into the factory, or routing both through one helper, would keep it in a single spot.

Reviewed via styfle's devbox

@styfle
styfle merged commit 29eaa88 into vercel:main Nov 5, 2026
16 checks passed
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 1.11.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants