Skip to content

WebKit: null, undefined and booleans stay what they were after a caught exception in DFG code (oven-sh/WebKit#772) - #44617

Closed
dylan-conway wants to merge 1 commit into
mainfrom
claude/webkit-772-catch-osr-entry
Closed

dylan-conway wants to merge 1 commit into
mainfrom
claude/webkit-772-catch-osr-entry

Conversation

@dylan-conway

Copy link
Copy Markdown
Member

What this fixes

Once a function had been compiled by the DFG, a local or an argument that is a double on the hot path came back as a number after a caught exception when it held null, true, undefined or false. Nothing throws and nothing is printed.

function save(record, bad) {
  const t = record.p; // a fraction, a timestamp, a price: a double on the hot path
  try {
    risky(bad);
  } catch {}
  return JSON.stringify({ t });
}
record.p, and risky() throws before after
null {"t":0} {"t":null}
true {"t":1} {"t":true}
undefined {"t":null} (NaN) {}
false {"t":0} {"t":false}

It is not specific to that shape. finally, using, for-of, async functions, generators, template strings, { v: t }, [t] and array[0] = t inside the handler, t || x, and integers past 2^31 in place of fractions all showed it.

Regression from #42002, in no release. With lazy catch liveness a handler that ran before the function first reached its DFG threshold, and not again before the compile, got a catch entrypoint with no type information for its operands. 1.4.2 is not affected by this one.

Not a regression, 1.4.2 has it too. A local that was in its TDZ whenever the handler was profiled ended up in the same state, whatever the option says. BUN_JSC_useLazyCatchLiveness=0 does not avoid it.

Cause and fix (oven-sh/WebKit#772)

The DFG takes a value without a prediction for code that never ran, and converts it without a check: a double use of such a value became ToNumber. A catch entrypoint could hand it one that is reachable.

Profiling a catch now records the empty value instead of treating it as "no sample", so one pass gives every operand a prediction, and the parser makes a catch entrypoint only when every operand has one. Otherwise the handler is treated like one that has yet to run: the exception exits to baseline. A try/catch inside a try/finally always has an empty operand (the completion value) and keeps its entrypoint.

Performance

A handler that ran only before its function got hot now has no catch entrypoint, like one that first runs after the function was optimized: its exceptions exit to baseline. In synthetic benchmarks that is the same or slightly faster for functions called many times, and 24% to 59% slower for one long-running loop that catches an exception early and then one in 50,000 to 500,000 iterations. Prettier over 3,241 files and tsc over a 290-file project make no catch entrypoint at all, before or after. Numbers in oven-sh/WebKit#772.

WebKit pin

WEBKIT_VERSION points at the preview build of oven-sh/WebKit#772 (5718a6ec579b plus its two commits). It moves to the merged commit before this merges.

Tests

test/js/bun/jsc-stress/fixtures/catch-osr-entry-operand-without-sample.js, the stress test of the engine PR for the values, listed in jsc-stress.test.ts. Five shapes: a local used after the catch, in a finally, in [t] inside the handler, an argument in [t] inside the handler, and a local that is in its TDZ when the handler is profiled. It compiles on the main thread so the tier-up point does not depend on scheduling.

The engine PR's second test asks whether a handler runs in optimized code, which needs $vm. It stays in JSTests.

…ht exception in DFG code (oven-sh/WebKit#772)

A local or an argument that is a double on the hot path came back as a number after a caught exception when it held null, true, undefined or false, once the DFG had compiled the function: JSON.stringify({ t }) wrote {"t":0} for null.

Pins the preview build of oven-sh/WebKit#772 and adds its stress test to the jsc-stress fixtures. The pin moves to the merged commit before this merges.
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