Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ses): Improve the performance of error serialization #1865

Closed
wants to merge 1 commit into from

Conversation

gibson042
Copy link
Contributor

Description

Noticed in passing: building a string by successive concatenation rather than pushing into an eventually-joined array is faster in both V8 (+63%) and XS (+39%), presumably because of rope optimizations.

$ esbench --eshost-option '-h V8,*XS*' '
  const { raw: StringRaw } = String;
  const [arrayIncludes, arrayMap, arrayPush, arrayJoin] = ["includes", "map", "push", "join"].map(
    m => Function.prototype.call.bind(Array.prototype[m]),
  );

  const useRaw = (template, ...args) =>
    StringRaw({ raw: template }, ...arrayMap(args, arg => `${arg}`));

  const useJoin = (template, ...args) => {
    const parts = [template[0]];
    for (let i = 0; i < args.length; i += 1) {
      const arg = args[i];
      const argStr = `${arg}`;
      arrayPush(parts, argStr, template[i + 1]);
    }
    return arrayJoin(parts, "");
  };

  const usePlus = (template, ...args) => {
    let result = `${template[0]}`;
    for (let i = 0; i < args.length; i += 1) {
      const arg = args[i];
      const argStr = `${arg}`;
      result += argStr + template[i + 1];
    }
    return result;
  };

  const useTemplates = (template, ...args) => {
    let result = `${template[0]}`;
    for (let i = 0; i < args.length; i += 1) {
      const arg = args[i];
      const argStr = `${arg}`;
      result += `${argStr}${template[i + 1]}`;
    }
    return result;
  };

  for (const [name, fn] of Object.entries({ useRaw, useJoin, usePlus, useTemplates })) {
    const actual = fn`a ${0.0} ${1e0}`;
    const expected = "a 0 1";
    if (actual === expected) continue;
    print(JSON.stringify({ actual, expected }));
    throw Error(`bad ${name}`);
  }' '{
    useRaw: "result = useRaw`problem ${0}: ${[`blue`, 42]}`",
    useJoin: "result = useJoin`problem ${0}: ${[`blue`, 42]}`",
    usePlus: "result = usePlus`problem ${0}: ${[`blue`, 42]}`",
    useTemplates: "result = useTemplates`problem ${0}: ${[`blue`, 42]}`",
  }'

Moddable XS

useRaw: 0.61 ops/ms
useJoin: 0.62 ops/ms
usePlus: 0.86 ops/ms
useTemplates: 0.72 ops/ms

V8

useRaw: 2.72 ops/ms
useJoin: 4.65 ops/ms
usePlus: 7.58 ops/ms
useTemplates: 7.35 ops/ms

Security Considerations

n/a

Scaling Considerations

👍

Documentation Considerations

n/a

Testing Considerations

all tests continue to pass

Upgrade Considerations

None beyond the usual concerns about changing vat bundles.

```console
$ esbench --eshost-option '-h V8,*XS*' '
  const { raw: StringRaw } = String;
  const [arrayIncludes, arrayMap, arrayPush, arrayJoin] = ["includes", "map", "push", "join"].map(
    m => Function.prototype.call.bind(Array.prototype[m]),
  );

  const useRaw = (template, ...args) =>
    StringRaw({ raw: template }, ...arrayMap(args, arg => `${arg}`));

  const useJoin = (template, ...args) => {
    const parts = [template[0]];
    for (let i = 0; i < args.length; i += 1) {
      const arg = args[i];
      const argStr = `${arg}`;
      arrayPush(parts, argStr, template[i + 1]);
    }
    return arrayJoin(parts, "");
  };

  const usePlus = (template, ...args) => {
    let result = `${template[0]}`;
    for (let i = 0; i < args.length; i += 1) {
      const arg = args[i];
      const argStr = `${arg}`;
      result += argStr + template[i + 1];
    }
    return result;
  };

  const useTemplates = (template, ...args) => {
    let result = `${template[0]}`;
    for (let i = 0; i < args.length; i += 1) {
      const arg = args[i];
      const argStr = `${arg}`;
      result += `${argStr}${template[i + 1]}`;
    }
    return result;
  };

  for (const [name, fn] of Object.entries({ useRaw, useJoin, usePlus, useTemplates })) {
    const actual = fn`a ${0.0} ${1e0}`;
    const expected = "a 0 1";
    if (actual === expected) continue;
    print(JSON.stringify({ actual, expected }));
    throw Error(`bad ${name}`);
  }' '{
    useRaw: "result = useRaw`problem ${0}: ${[`blue`, 42]}`",
    useJoin: "result = useJoin`problem ${0}: ${[`blue`, 42]}`",
    usePlus: "result = usePlus`problem ${0}: ${[`blue`, 42]}`",
    useTemplates: "result = useTemplates`problem ${0}: ${[`blue`, 42]}`",
  }'
#### Moddable XS
useRaw: 0.61 ops/ms
useJoin: 0.62 ops/ms
usePlus: 0.86 ops/ms
useTemplates: 0.72 ops/ms

#### V8
useRaw: 2.72 ops/ms
useJoin: 4.65 ops/ms
usePlus: 7.58 ops/ms
useTemplates: 7.35 ops/ms
```
@erights
Copy link
Contributor

erights commented Dec 1, 2023

and XS (+39%), presumably because of rope optimizations.

But XS doesn't use ropes.

@phoddie , shouldn't we generally use repeated array push + join in XS rather than repeated string appends? Does it make sense the latter could be faster?

@phoddie
Copy link

phoddie commented Dec 1, 2023

@erights – Correct, XS doesn't use ropes. Perhaps for strings created for a smaller number of items, using string concatenation is faster but as the number of items grows, the array push + join approach is faster?

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

Given @phoddie 's response at #1865 (comment) , the pattern used in this PR is in general a bad precedent compared to the status quo. Since the speed advantage on XS is only for these small cases, I suspect that this specific speed advantage shouldn't make a practical difference, especially since errors should be absent from fast-paths in general. But if there's a reason why this specific speed advantage for errors is of significant practical benefit, that would be interesting. Is there?

If not, I suggest that this PR be closed in favor of the status quo.

@erights
Copy link
Contributor

erights commented Dec 30, 2023

If not, I suggest that this PR be closed in favor of the status quo.

Close?

@erights erights self-requested a review December 30, 2023 21:48
@gibson042
Copy link
Contributor Author

Given the above, it seems like small-input performance improvements do not provide sufficient benefit to justify introducing the pattern.

@gibson042 gibson042 closed this Jan 5, 2024
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.

3 participants