feat(ses): finite deep stacks, on by default

[erights]

The deep stacks continue to use the assert.note mechanism to associate an error with previous stack-capturing errors. The unbounded leak that caused us to turn deep stacks off was really the unbounded memory of error-note-graphs.

Prior to this PR, the line disabling deep stacks by default was commented

// ... We intend to change the default after verifying that having
// the feature enabled in production does not cause memory to leak.

This PR makes the error-note storage into a finite budget LRU cache, defaulting to a budget of 1000. With the memory leak fixed, track-turns.js is changed to enable deep stacks by default. Measurements will still be needed to see what the cost is, and what is a reasonable budget.

[erights]

@kriskowal if you could have a look at this, that'd be great. CI is giving me a jackpot of errors, all of which seem rooted in

Error running main: TypeError: Cannot read property 'length' of undefined
    at main (file:///home/runner/work/endo/endo/packages/ses/scripts/bundle.js:28:46)

where the immediately relevant code in bundle.js is

  const { code: terse } = terser.minify(bundle, {
    mangle: false,
    keep_classnames: true,
  });

  console.log(`Bundle size: ${bundle.length} bytes`);
  console.log(`Minified bundle size: ${terse.length} bytes`);

However, this PR itself is clearly completely independent of bundling. I don't see how it could be affected. Any ideas?

[gibson042]

The unbounded leak that caused us to turn deep stacks off was really the unbounded memory of error-note-graphs.

Huh, really? I would have expected takeNoteLogArgsArray to free that... was nothing calling it?

[erights]

The unbounded leak that caused us to turn deep stacks off was really the unbounded memory of error-note-graphs.

Huh, really? I would have expected takeNoteLogArgsArray to free that... was nothing calling it?

Only for errors that are actually reported. The fatal leak were for errors that could eventually be potentially reported, but were not.

[erights]

PTAL

[erights]

I also changed the default budget from 10_000 to 1000 because, in the absence of measurement, I'd really like to avoid guessing too high. If too low, it is still overall better than the status quo, with deep stacks default turned off. And we can always increase it later.

We should also consider configuring the budget with an environment option.

[erights]

Worked around stale tooling. Bug at #1514 . (@kriskowal , thanks!)

[erights]

Re #1513 (comment)

I reduced the note-args budget again, this time to 100. At 1000 I ran into one problem:

// Bizarrely, if set to 1000 or 10_000, captp/test/test-gc.js
// test 'test loopback gc' fails ONLY ON WINDOWS.
// TODO: Figure out why, and either special case or fix.
// TODO: Once captp/test/test-gc.js doesn't stop us,
// set this default back to a more reasonable number.
// TODO: make this default configurable by an environment option.
const defaultLogArgsBudget = 100;

[erights]

Given the nature of this PR, before merging, it is worth checking the effect on performance. The following first crude test is consistent enough to show we're ok, or even a bit better!

#1515 , identical to current master, has the following timings from CI

Compare with the timings of this PR under CI. Bizarrely, this PR is always the same or faster. (ignoring the failed test)

[erights]

At 100 the same test now fails or passes intermittently, which is even worse!

[erights]

Re #1513 (comment) , #1516 fixed the immediate problem, the OS-dependent behavior. However, this PR still changes the counts, for reasons we (@michaelfig and I) do not yet precisely understand but believe are fine for now.

@michaelfig, I changed those lines as we discussed but also added a TODO

  // TODO(mfig,erights): explain why #1513 changed these counts from 3 to 2
  t.is(getFarStats().sendCount.CTP_DROP, 2);
  t.is(getNearStats().recvCount.CTP_DROP, 2);