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

feat!: switch back to protons #468

Merged
merged 9 commits into from Feb 10, 2024
Merged

feat!: switch back to protons #468

merged 9 commits into from Feb 10, 2024

Conversation

tuyennhv
Copy link
Contributor

@tuyennhv tuyennhv commented Oct 27, 2023

Motivation
Switch from protobufjs to protons

Description

While decode flow is comparable or a little bit faster, encode flow is ~1.5x slower

  protobuf
    ✔ decode Attestation message using protobufjs                          3318731 ops/s    301.3200 ns/op       396763 runs    120 s
    ✔ encode Attestation message using protobufjs                          2515603 ops/s    397.5190 ns/op       300351 runs    120 s
  protobuf
    ✔ decode Attestation message using protons-runtime 5.1.0               3996387 ops/s    250.2260 ns/op        -     478012 runs    120 s
    ✔ encode Attestation message using protons-runtime 5.1.0               1667211 ops/s    599.8040 ns/op        -     199077 runs    120 s

cc @wemeetagain

Closes #453
Closes #318

TODOs
Test with lodestar to see how it effect the performance

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Attention: 202 lines in your changes are missing coverage. Please review.

Comparison is base (e39b2e2) 81.39% compared to head (3060f20) 78.52%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/message/rpc.ts 75.46% 184 Missing ⚠️
src/index.ts 79.22% 16 Missing ⚠️
src/metrics.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   81.39%   78.52%   -2.88%     
==========================================
  Files          48       46       -2     
  Lines       12333    10868    -1465     
  Branches     1303     1058     -245     
==========================================
- Hits        10039     8534    -1505     
- Misses       2294     2334      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tuyennhv
Copy link
Contributor Author

this branch

    ✔ decode Attestation message 300 bytes                                 3965437 ops/s    252.1790 ns/op        -     236769 runs   60.1 s
    ✔ encode Attestation message 300 bytes                                 1710750 ops/s    584.5390 ns/op        -     101726 runs   60.0 s
    ✔ decode SignedBeaconBlock message 70000 bytes                         4061491 ops/s    246.2150 ns/op        -     242494 runs   60.1 s
    ✔ encode SignedBeaconBlock message 70000 bytes                        87208.34 ops/s    11.46679 us/op        -       5187 runs   60.0 s
    ✔ decode SignedBeaconBlock message 140000 bytes                        4217559 ops/s    237.1040 ns/op        -     251807 runs   60.1 s
    ✔ encode SignedBeaconBlock message 140000 bytes                       64996.40 ops/s    15.38547 us/op        -       3869 runs   60.1 s
    ✔ decode SignedBeaconBlock message 210000 bytes                        4347410 ops/s    230.0220 ns/op        -     259492 runs   60.1 s
    ✔ encode SignedBeaconBlock message 210000 bytes                       52970.20 ops/s    18.87854 us/op        -       3155 runs   60.1 s
    ✔ decode SignedBeaconBlock message 280000 bytes                        4504423 ops/s    222.0040 ns/op        -     268977 runs   60.0 s
    ✔ encode SignedBeaconBlock message 280000 bytes                       43202.80 ops/s    23.14665 us/op        -       2572 runs   60.1 s

on master

  ✔ decode Attestation message 300 bytes                                 3412981 ops/s    292.9990 ns/op        -     203542 runs   60.0 s
    ✔ encode Attestation message 300 bytes                                 2277552 ops/s    439.0680 ns/op        -     135393 runs   60.0 s
    ✔ decode SignedBeaconBlock message 70000 bytes                         3336959 ops/s    299.6740 ns/op        -     198990 runs   60.0 s
    ✔ encode SignedBeaconBlock message 70000 bytes                        231922.1 ops/s    4.311793 us/op        -      13791 runs   60.0 s
    ✔ decode SignedBeaconBlock message 140000 bytes                        3267536 ops/s    306.0410 ns/op        -     194835 runs   60.0 s
    ✔ encode SignedBeaconBlock message 140000 bytes                       81215.86 ops/s    12.31287 us/op        -       4830 runs   60.0 s
    ✔ decode SignedBeaconBlock message 210000 bytes                        3535280 ops/s    282.8630 ns/op        -     210959 runs   60.1 s
    ✔ encode SignedBeaconBlock message 210000 bytes                       61267.33 ops/s    16.32191 us/op        -       3650 runs   60.1 s
    ✔ decode SignedBeaconBlock message 280000 bytes                        3575566 ops/s    279.6760 ns/op        -     213582 runs   60.1 s
    ✔ encode SignedBeaconBlock message 280000 bytes                       50704.93 ops/s    19.72195 us/op        -       3019 runs   60.1 s

with latest benchmark (with different lengths of messages)

  • decode does not depend on message length, this branch is consistently 20%-25% better than master
  • encode: this branch is 13% - 33% slower than master, the bigger the message, the smaller the difference

achingbrain added a commit to achingbrain/gossipsub-js that referenced this pull request Jan 30, 2024
@achingbrain
Copy link
Collaborator

@tuyennhv what's the status of this PR? Could it be moved forward?

@tuyennhv
Copy link
Contributor Author

sorry @achingbrain for the delay, I just rebased against master, testing it with lodestar now cc @wemeetagain

@tuyennhv
Copy link
Contributor Author

tuyennhv commented Feb 1, 2024

somehow rss in this branch is consistently higher than with gossipsub v11.1.0 when I test on different nodes

# this branch v11.1.0
holesky 1k 12.5GB 9GB
mainnet 9.5GB 7GB
holesky 64 15GB 10GB

@achingbrain
Copy link
Collaborator

That's interesting, protons is synchronous and stateless so any memory it uses would be garbage collectable immediately.

That said, it's going to have a different memory usage pattern to protobuf.js so it could in turn affect how other components use memory.

Do you have a view on what is taking up the extra space?

@tuyennhv
Copy link
Contributor Author

tuyennhv commented Feb 6, 2024

That's interesting, protons is synchronous and stateless so any memory it uses would be garbage collectable immediately.

@achingbrain how is it different from protobuf.js? I thought protobuf.js is synchronous and stateless too

That said, it's going to have a different memory usage pattern to protobuf.js so it could in turn affect how other components use memory.

Do you have a view on what is taking up the extra space?

I think it's worth to compare https://github.com/protobufjs/protobuf.js/blob/master/src/reader.js and https://github.com/ipfs/protons/blob/main/packages/protons-runtime/src/utils/reader.ts . From a quick look it shares a lot of similarities

@achingbrain
Copy link
Collaborator

achingbrain commented Feb 6, 2024

how is it different from protobuf.js

It isn't, the reader/writer parts of protons are from protobuf.js, ported to ESM.

From a quick look it shares a lot of similarities

Yeah, it's basically the same code which is why it's surprising the RSS is larger.

For the same workload the memory behaviour should be comparable. I don't have a view on the testing you've been doing with Lodestar so I can only assume the workload is different.

Would it be possible for you to record the inputs and outputs of the protobuf.js usage in the app, (e.g. write the buffers being decoded/objects being encoded out to a file), then we could replay them without the rest of lodestar present and observe the memory usage?

@tuyennhv
Copy link
Contributor Author

tuyennhv commented Feb 7, 2024

just found out I compared orange to apple, I used service deployment to test this branch and compared it to docker deployment instances, and the service deployments always have higher RSS somehow. So we don't need to worry about RSS, I'll need 1-2 more days to make sure number of mesh peers are the same @achingbrain

@tuyennhv
Copy link
Contributor Author

tuyennhv commented Feb 8, 2024

mesh peers are similar to v11.x.x in lodestar, ready for review

Screenshot 2024-02-09 at 06 36 20

@tuyennhv tuyennhv marked this pull request as ready for review February 8, 2024 23:36
@tuyennhv tuyennhv requested a review from a team as a code owner February 8, 2024 23:36
Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

looks good to me

*/
export function decodeRpc (bytes: Uint8Array, opts: DecodeRPCLimits): IRPC {
export function decodeRpc (bytes: Uint8Array, opts: DecodeRPCLimits): RPC {
Copy link
Member

Choose a reason for hiding this comment

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

I think protons runtime now can limit fields while decoding.
Might be worth looking into as a followup PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can give it a try thanks @wemeetagain 👍

@tuyennhv tuyennhv marked this pull request as draft February 9, 2024 05:24
@tuyennhv
Copy link
Contributor Author

I've tested this branch for ~18h: mesh peers are good (since ~6h30)

Screenshot 2024-02-10 at 07 21 13

external memory is unstable, not sure if it's an issue, just something to notice for later reference

Screenshot 2024-02-10 at 07 22 15

@tuyennhv tuyennhv marked this pull request as ready for review February 10, 2024 00:23
@wemeetagain wemeetagain merged commit 18955dc into master Feb 10, 2024
9 checks passed
@wemeetagain wemeetagain deleted the tuyen/protons_7 branch February 10, 2024 04:15
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.

vuln prototype pollution in dependency protobufjs Add protons back
4 participants