assert: fix slow error message generation for minified code#62338
assert: fix slow error message generation for minified code#62338Felipeness wants to merge 2 commits intonodejs:mainfrom
Conversation
When assert.ok(falsy) is called in minified/bundled code (common in Lambda deployments via Webpack/Terser), the error message generation can take seconds or even minutes. The getFirstExpression function tokenizes the entire source line with acorn to extract the failing expression, but for minified files the source line is the entire file content (potentially megabytes of code on a single line). Fix by windowing the tokenization: when the source line exceeds 2048 characters, extract a ~1024 character window around the target column instead of tokenizing the entire line. The window uses semicolons as safe cut points to give acorn valid-ish input. A try-catch is added to gracefully handle cases where the windowed code starts mid-token. This preserves the detailed error message (including member access like assert.ok) while bounding tokenization work to a constant size regardless of file length. Fixes: nodejs#52677
nodejs-github-bot
added
errors
- Return undefined from catch block in getFirstExpression when acorn throws on malformed windowed code, instead of falling through and returning garbage from partially-tokenized input. - Reduce test fixture size from 100K variable declarations (~2.8MB) to ~3000 chars (just above kMaxSourceLineLength threshold). This still exercises the windowing path without stressing the filesystem. - Replace fixed 10s timeout as correctness assertion with elapsed time measurement via process.hrtime.bigint(). The test now asserts completion in under 10 seconds, which is more robust on slow CI.
Do you have a reproduction case that demonstrates it taking minutes? I find that extremely difficult to believe. Specifically, acorn is generally quite fast and there are hard practical limits already in place that would prevent it from taking "minutes". I think at best this would shave a couple hundred milliseconds off an error path, which is of questionable value given the added complexity. |
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
Not really seeing this as a valid optimization. See prior comment for reasoning.
|
Hey @jasnell, fair question. I'm working on putting together a proper reproduction case for the timing claim. Will update here once I have it. Been a bit sidetracked dealing with Windows build issues on another PR, but this is on my list. Thanks for the patience. |
3 participants
Summary
When
assert.ok(falsy)is called in minified/bundled code (common in Lambda deployments via Webpack/Terser), the error message generation can take seconds to minutes. ThegetFirstExpressionfunction inlib/internal/errors/error_source.jstokenizes the entire source line with acorn to extract the failing expression, but for minified single-line files the source line is the entire file (potentially megabytes).This PR fixes the performance issue by windowing the tokenization:
assert.ok) is preservedBenchmark results (tested manually)
Test plan
test/parallel/test-assert-long-line-perf.jswith two tests:assert.okdoes not hang on a large synthetic minified file (100K declarations + assert)ok(false))test-assert-first-line.jstest (which testsassert-long-line.js, a 9.4KB single-line fixture) still produces the correct error message with windowing enabledassert.ok(false),assert['ok'](false), and plain;-padded assert callsFixes: #52677
Jira: N/A (open source contribution)