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(exchange): Add pre-allocating the buffer for reading in HeaderCodec #64

Merged
merged 20 commits into from
Sep 12, 2023

Conversation

fl0rek
Copy link
Member

@fl0rek fl0rek commented Sep 11, 2023

closes #43

Read up to first 10 bytes (max length of the length delimiter varint) in a loop until prost::decode_length_delimiter is able to figure out message length. Then, when reading, preallocate the buffer and read the exact number of bytes needed.

@fl0rek fl0rek force-pushed the feat/header-request-pre-alloc branch from 311d80d to 5ea68a5 Compare September 11, 2023 09:09
@oblique
Copy link
Member

oblique commented Sep 11, 2023

read_response should implement the same mechanism. I suggest implementing a generic read_raw_message that returns the raw bytes and then use it in both.

node/src/exchange.rs Outdated Show resolved Hide resolved
@fl0rek fl0rek changed the title feat(exchange): Add pre-allocating header request receive buffer based on length delimiter feat(exchange): Add pre-allocating the buffer for reading in HeaderCodec Sep 11, 2023
@fl0rek fl0rek force-pushed the feat/header-request-pre-alloc branch 2 times, most recently from c84340d to 5ea68a5 Compare September 11, 2023 12:49
Copy link
Member

@zvolin zvolin 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.

Recently we've been discussing the proptest as guys in sovereign seems using this alot and this looks like a perfect use for it. We could add fuzz tests for the codec but I leave it up to you guys, @fl0rek @oblique

node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
@fl0rek fl0rek force-pushed the feat/header-request-pre-alloc branch from 5809524 to 49cc72f Compare September 12, 2023 07:46
@fl0rek fl0rek force-pushed the feat/header-request-pre-alloc branch from 49cc72f to f18a99d Compare September 12, 2023 07:57
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
fl0rek and others added 3 commits September 12, 2023 12:23
Co-authored-by: Yiannis Marangos <psyberbits@gmail.com>
Co-authored-by: Yiannis Marangos <psyberbits@gmail.com>
node/Cargo.toml Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/exchange.rs Outdated Show resolved Hide resolved
node/src/p2p.rs Outdated Show resolved Hide resolved
@fl0rek fl0rek force-pushed the feat/header-request-pre-alloc branch from d54791a to 559569d Compare September 12, 2023 12:06
node/src/exchange.rs Outdated Show resolved Hide resolved
Copy link
Member

@oblique oblique left a comment

Choose a reason for hiding this comment

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

lgtm

@oblique oblique merged commit ae6f0ff into eigerco:main Sep 12, 2023
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 9, 2024
@zvolin zvolin mentioned this pull request Jan 12, 2024
@fl0rek fl0rek deleted the feat/header-request-pre-alloc branch July 22, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants