erights
commented
1 year ago Good point about time travel. In light of that, since we're better off with this than without, I'll merge. Thx.
Closed erights closed 1 year ago
erights
commented
1 year ago Good point about time travel. In light of that, since we're better off with this than without, I'll merge. Thx.
gibson042
commented
1 year ago @erights Have we considered instead creating a call-specific stack of labels at the matches/mustMatch entry points and passing it down such that there are no redundant calls to checkMatches or even nested try..catch contexts?
const mustMatch = (specimen, patt, label = undefined) => {
- let innerError;
+ const labels = label === undefined ? [] : [label];
+ const labelCount = labels.length;
try {
- if (checkMatches(specimen, patt, identChecker, undefined)) {
- return;
- }
+ const result = checkMatches(specimen, patt, assertChecker, labels);
+ labels.length === labelCount && labels[0] === label ||
+ Fail`internal: unexpected final label stack ${labels}`;
+ return result;
} catch (er) {
- innerError = er;
+ // const wrapError = (err, labels) => {
+ // for (let i = labels.length - 1; i >= 0; i -= 1) {
+ // let label = labels[i];
+ // cf. `bestEffortStringify`
+ // if (typeof label === 'number' || typeof label === 'string' && label[0] === '[') {
+ // label = `[${label}]`;
+ // }
+ // const outerErr = assert.error(`${label}: ${err.message}`);
+ // assert.note(outerErr, X`Caused by ${err}`);
+ // err = outerErr;
+ // }
+ // return err;
+ // };
+ throw wrapError(er, labels);
}
- // should only throw
- checkMatches(specimen, patt, assertChecker, label);
- const outerError = assert.error(
- X`internal: ${label}: inconsistent pattern match: ${q(patt)}`,
- );
- if (innerError !== undefined) {
- assert.note(outerError, X`caused by ${innerError}`);
- }
- throw outerError;
};checkMatches to extend/contract a shared stack of labels rather than being called with new labels in nested try..catch contextserights
commented
1 year ago Have we considered instead creating a call-specific stack of labels at the matches/mustMatch entry points and passing it down such that there are no redundant calls to checkMatches or even nested try..catch contexts?
IIUC, no. Not sure I follow, but I certainly like the difference in reported stacks!
Rather than try to read a diff or manually apply it, could you turn this into a PR for me to review?
gibson042
commented
1 year ago Rather than try to read a diff or manually apply it, could you turn this into a PR for me to review?
Yes, but first I wanted to try measuring actual performance. The improvement appears to be small but real.
esbench -n 100 --repetitions 20 --tmp --eshost-option '-h V8,*XS*' \
--init-file <(printf '
import "@endo/init";
import { M, matches, mustMatch } from "@endo/patterns";
Object.assign(globalThis, { M, matches, mustMatch });
' | npx rollup -p @rollup/plugin-node-resolve -f iife) \
'
const { M, matches, mustMatch } = globalThis;
const patt = harden({arr: ["foo", ["bar", "BAR"], [M.string(), "qux"]]});
' '{
"pass": `result = matches(harden({arr: ["foo", ["bar", "BAR"], ["qux", "qux"]]}), patt);`,
"fail": `result = matches(harden({arr: ["foo", ["bar", "BAR"], ["qux", "quux"]]}), patt);`,
}'Before:
#### Moddable XS
pass: 0.13 ops/ms
fail: 0.13 ops/ms
#### V8
console.warn: Removing intrinsics.Promise.withResolvers
pass: 0.95 ops/ms
fail: 1.05 ops/msAfter:
#### Moddable XS
pass: 0.15 ops/ms
fail: 0.15 ops/ms
#### V8
console.warn: Removing intrinsics.Promise.withResolvers
pass: 1.02 ops/ms
fail: 1.08 ops/ms
By passing
labelhere, we missed an intended optimization byapplyLabelingError, and instead created needless stackframes with a try/catch for every level of a recursive pattern match, even for themustMatchsuccess case. With theundefined, whenmustMatchsucceeds, we should avoid all these. WhenmustMatchfails, we'll cut the number of these in half.I do NOT know whether or not this change will make a significant performance difference. I also do not know if the significance will be different on v8 vs XS. I want to use this as my benchmarking + profiling "hello world" exercise, to get to know our performance measurement tooling.
This PR itself only makes the change. It does not measure anything.