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

chore: switch to pure-go zstd decoder for snapshot imports #12857

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

masih
Copy link
Member

@masih masih commented Jan 29, 2025

Related Issues

Proposed Changes

Additional Info

The benchmarking performed as part of #12852 shows that the pure-go implementation of zstd decoder is fast-enough for snapshot import.

Switch to it motivated by the desire to reduce CGO dependencies across Lotus.

Fixes #12852

Checklist

Before you mark the PR ready for review, please make sure that:

The benchmarking performed as part of #12852 shows that the pure-go
implementation of zstd decoder is fast-enough for snapshot import.

Switch to it motivated by the desire to reduce CGO dependencies across
Lotus.

Fixes #12852
@masih masih requested a review from Stebalien January 29, 2025 16:10
go.sum Show resolved Hide resolved
@masih masih self-assigned this Jan 29, 2025
@BigLep
Copy link
Member

BigLep commented Jan 29, 2025

Thanks for the work here @masih . Quick question: why is there a release/backport label? I believe there is no active release train to backport into. I assume the next release (assuming not a rushed security update) will branch from master.

@masih
Copy link
Member Author

masih commented Jan 29, 2025

why is there a release/backport label? I believe there is no active release train to backport into. I assume the next release (assuming not a rushed security update) will branch from master.

Yep, no need to rush this one. I understand the label to mean "please include in the next release".

Please let me know if I am misinterpreting the label.

@Stebalien
Copy link
Member

It means to backport to an existing release candidate. Everything in master will automatically go into the next release unless the release process is already underway.

@masih
Copy link
Member Author

masih commented Jan 29, 2025

It means to backport to an existing release candidate. Everything in master will automatically go into the next release unless the release process is already underway.

I was explicitly explained to that for anything to end up in a release it needs that label. I am also known to missunderstand things at times.

Thank you for clarifying this for me, Steven.

@Stebalien
Copy link
Member

Yeah, I'm guessing the intent of that statement was "if it needs to end up in this release that's currently baking".

@rvagg
Copy link
Member

rvagg commented Jan 30, 2025

Current lotus to import a mainnet snapshot into a fresh repo:

real    16m47.633s
user    20m44.787s
sys     4m1.878s

Same data, this branch:

real    16m24.879s
user    23m23.562s
sys     4m31.122s

So .. I'm not really sure what to make of the reduction in real time there (I was doing other things during both of these runs so likely skewed) but we can at least say that (1) the pure-go implementation has to work harder to get the job done, and that (2) the bulk of the cost of snapshot import is almost entirely in Badger so zstd effort isn't really a factor here.

@masih masih enabled auto-merge (squash) January 30, 2025 09:00
@masih masih removed the request for review from Stebalien January 30, 2025 09:00
@masih masih merged commit aa529a9 into master Jan 30, 2025
96 checks passed
@masih masih deleted the masih/no-cgo-zstd branch January 30, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

Compare the performane of DataDog/zstd with the pure-go implementation by klauspost/compress
5 participants