Remove protobufjs in favor of protons-runtime #60
Conversation
|
@talentlessguy you might gain more savings switching to protons and protons-runtime instead? It would dudupe with |
|
Note: I think in general I'd prefer to switch to |
thanks for the suggestion |
|
FWIW, last time I ran the benchmark suite, protons was just over 20% faster than protobuf-es when serializing/deserializing - https://github.com/ipfs/protons/tree/main/packages/protons-benchmark#usage These kind of performance characteristics are very important to high traffic deployments like Lodestar so it's unlikely to be switched away from elsewhere. |
do you mean protobufjs, and not protons? |
|
No, I mean protons: 
protobuf.js is also quite a bit faster than protobuf-es but it's CJS and the stack is ESM-only so that rules it out. |
|
I wonder how hard it would be to replace protobuf-es with protons, will give it a shot rn |
|
I guess the only thing is that protons compiles protobuf definitions to TypeScript and this is still a js project. |
talentlessguy
changed the title
|
@achingbrain is the benchmark encode or decode? We're only encode in this repo. |
src/codec.js
Outdated
| Data: content.byteLength > 0 ? content : undefined, | ||
| filesize: content.byteLength, | ||
| Data: content.byteLength > 0 ? content : EMPTY_BUFFER, | ||
| filesize: BigInt(content.byteLength), |
There was a problem hiding this comment.
Does the dag-pb encoder support bigint?
There was a problem hiding this comment.
I'm not sure about dag-pb, is it relevant to the PR?
| Data: content.byteLength > 0 ? content : undefined, | ||
| filesize: content.byteLength, | ||
| Data: content.byteLength > 0 ? content : EMPTY_BUFFER, | ||
| filesize: content.length === 0 ? Object.assign(0n, { __forceEncode: true }) : BigInt(content.length), |
There was a problem hiding this comment.
Why do we need __forceEncode here?
There was a problem hiding this comment.
Adding these undefined fields and setting __forceEncode seems to be repeated often - shouldn't it be moved to encodePB so we get less change in other places?
There was a problem hiding this comment.
this is because protons-runtime ignores 0n because it thinks it's a default and because of that doesn't include it in the protobuf
There was a problem hiding this comment.
Protons is a proto3 implementation though it tries it's best with proto2. The big difference is there's no "required" modifier any more because the authors considered it harmful.
If you want a value to always be in the protobuf in proto3 you should mark it optional and set a value. This is compatible on the wire with required in proto2.
E.g. these will result in the same bytes:
syntax = "proto2";
message Derp {
required int64 Value = 1;
}syntax = "proto3";
message Derp {
optional int64 Value = 1;
}You can see the proto3 definition ipfs-unixfs uses here and the bytes generated are compatible with older proto2 parsers.
There was some discussion about this on the libp2p specs repo a while back which goes over the various pros and cons and how to maintain backwards compatibility - libp2p/specs#465
There was a problem hiding this comment.
So what's the best solution? migrating js-unixfs to use proto3 or just keep it the way it is?
This closes #31
Switches to protons-runtime
All tests pass now