feat: fast ArrayBuffer encoding #566
Conversation
ninegua
changed the title
packages/candid/src/idl.test.ts
Outdated
| @@ -152,6 +152,20 @@ test('IDL encoding (tuple)', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| test('IDL encoding (arraybuffer)', () => { | |||
| // ArrayBuffer, encode only. | |||
| testEncode( | |||
There was a problem hiding this comment.
I didn't change the decoding behavior because it will be a breaking change.
But I will suggest decoding should be changed too. What do people think?
There was a problem hiding this comment.
Commit a1e97d6 adds a fast decoding path too. This changes the return type from Array to ArrayBuffers, and will likely break user code when they upgrade.
ninegua
requested review from
chenyan-dfinity,
krpeacock and
ericswanson-dfinity
| ); | ||
| test_( | ||
| IDL.Vec(IDL.Nat64), | ||
| new BigUint64Array([BigInt(0), BigInt(1), BigInt(1) << BigInt(60), BigInt(13)]), |
There was a problem hiding this comment.
Nice! I didn't know BigUint64Array exists.
| // Special case for ArrayBuffer | ||
| const bits = this._type instanceof FixedNatClass ? this._type.bits : (this._type instanceof FixedIntClass ? this._type._bits : 0); | ||
| return (ArrayBuffer.isView(x) && bits == (x as any).BYTES_PER_ELEMENT * 8) | ||
| || (Array.isArray(x) && x.every(v => this._type.covariant(v))); | ||
| } | ||
|
|
||
| public encodeValue(x: T[]) { | ||
| const len = lebEncode(x.length); | ||
| if (this._blobOptimization) { |
There was a problem hiding this comment.
Seems _blobOptimization is not needed anymore?
There was a problem hiding this comment.
It is still needed if people pass plain array in.
If we remove this, then old code will probably get worse performance unless they also switch from Array to Uint8Array.
There was a problem hiding this comment.
Maybe add a comment in the code. I think eventually, people want to use typedArray for large vector anyway.
|
|
||
| if (this._type instanceof FixedNatClass) { | ||
| if (this._type.bits == 8) { | ||
| return new Uint8Array(b.read(len)) as unknown as T[]; |
There was a problem hiding this comment.
Is this a breaking change? If so, we need to document it.
There was a problem hiding this comment.
I added a doc string to VecClass.
Not sure how to add to CHANGELOG.md since there is none in this repo.
Co-authored-by: Yan Chen <48968912+chenyan-dfinity@users.noreply.github.com>
Description
At the moment trying to encode large binary data is very inefficient because one has to convert them to Array first before passing to the IDL encode function.
In fact trying to convert 100MB ArrayBuffer to Array will result in OOM in Node.js.
This patch adds a special case to type check and allows directly passing ArrayBuffer (e.g. Uint8Array) to the encode function and solves the problem.
How Has This Been Tested?
A test case is added.
Checklist: