perf(marshal): Replace XS-expensive string operations #2001
Conversation
gibson042
force-pushed
the
gibson-2024-01-passable-perf
branch
from
d74ddb8 to
d02889a
Compare
| const tagpayload = decodeArray(encoded.substring(1), decodePassable); | ||
| // Skip the ":" inside `decodeArray` to avoid slow `substring` in XS. | ||
| // https://github.com/endojs/endo/issues/1984 | ||
| const tagpayload = decodeArray(encoded, decodePassable, 1); |
There was a problem hiding this comment.
| const tagpayload = decodeArray(encoded, decodePassable, 1); | |
| const tagPayload = decodeArray(encoded, decodePassable, 1); |
Or do you have a convention for when you avoid midcaps? Why? I also notice keysvals above.
There was a problem hiding this comment.
No, it was preexisting (and git blamegit praise) identifies introduction in the landmark Agoric/agoric-sdk#4468). But worth fixing here, which I have done for both sites.
There was a problem hiding this comment.
(FWIW I'm surprised I did that and have no idea why I did. Glad to see it fixed, thanks!)
| Fail`Unexpected character after u0001 escape: ${c2}`; | ||
| elemChars.push(c2); | ||
| inEscape = true; | ||
| // eslint-disable-next-line no-continue |
There was a problem hiding this comment.
Not a comment on this PR, but curious: Do you think we should turn off warning on continue? I don't see the value in it.
There was a problem hiding this comment.
I would love to disable that warning; continue is as useful as break and early return and I don't see any value in discouraging it.
| } | ||
| elemChars.length === 0 || Fail`encoding terminated early: ${encoded}`; | ||
| !inEscape || Fail`unexpected end of encoding ${encoded.slice(skip)}`; |
There was a problem hiding this comment.
Good point! Expensive operations in that position are fine.
gibson042
force-pushed
the
gibson-2024-01-passable-perf
branch
from
d02889a to
3745bab
Compare
Both are in the same file and included in this PR (and in fact most of the changes affect decoding). |
Ref #1982 Speeds up XS by 21% decoding a representation of the following CopyRecord: ``` { primitives: [undefined, null, true, false, 0, 1, 0n, 1n, "foo", Symbol.for("bar")], arrays: [[], [{}]], records: { empty: {}, singular: { array: [] } }, } ``` (and speeds up decoding a CopyArray containing 10 copies thereof by about 100%)
gibson042
force-pushed
the
gibson-2024-01-passable-perf
branch
from
3745bab to
dac5bc0
Compare
Oops. I mean |
|
Ah, yes. I'll do that in the next round of updates to #1594 this week. |
Ref #1982
Description
Speeds up by 21% decoding a representation of the following CopyRecord:
(and speeds up decoding a CopyArray containing 10 copies thereof by about 100%)
No significant effects in Node.js.
Security Considerations
n/a
Scaling Considerations
As stated on the tin.
Documentation Considerations
n/a
Testing Considerations
n/a
Compatibility Considerations
n/a
Upgrade Considerations
n/a