KillingSpark / zstd-rs

zstd-decoder in pure rust
MIT License
253 stars 34 forks source link

remove derive_more #60

Closed xd009642 closed 5 months ago

xd009642 commented 5 months ago

First appologies for the size of this PR I can break it up if desired into a PR per file or something less giant :sweat_smile:

This PR removes derives_more and thus the dependency on syn/quote etc. Both versions of syn take approximately 25-30s for a clean build on my system and a lot of crates currently have syn 1.x and 2.x in the dependency tree. Because of syn 2.x being blocked on a 1.0.0 release of derive_more (which seems blocked for other reasons) I suggested removing derive_more and replacing with the handrolled implementations oin #55 .

Initially I started doing this in src/frame.rs manually until I realised just how many impls there were to replace. After that I took the approach of using cargo expand and cleaning up the names and impled code to look less generated.

One thing changed - all the display impls previously had #[inline] as a hint I didn't persist that but I could add it in just to ensure performance doesn't change.

Ignoring dev-dependencies for a user of this project the dependency tree would look like:

ruzstd v0.6.0 (/home/daniel/personal/zstd-rs)
├── byteorder v1.5.0
└── twox-hash v1.6.3
    ├── cfg-if v1.0.0
    └── static_assertions v1.1.0

Whereas before it was:

ruzstd v0.6.0 (/home/daniel/personal/zstd-rs)
├── byteorder v1.5.0
├── derive_more v0.99.17 (proc-macro)
│   ├── proc-macro2 v1.0.82
│   │   └── unicode-ident v1.0.12
│   ├── quote v1.0.36
│   │   └── proc-macro2 v1.0.82 (*)
│   └── syn v1.0.109
│       ├── proc-macro2 v1.0.82 (*)
│       ├── quote v1.0.36 (*)
│       └── unicode-ident v1.0.12
└── twox-hash v1.6.3
    ├── cfg-if v1.0.0
    └── static_assertions v1.1.0
KillingSpark commented 5 months ago

Thanks for the effort! I'll review it soon :)

xd009642 commented 5 months ago

There we go, clippy should now be happy!

xd009642 commented 5 months ago

And sorry about that fat fingered what was meant to be a simple change, all fixed now and I've ran cargo-hack and clippy locally and it passes as expected

KillingSpark commented 5 months ago

Thanks a lot! :)

xd009642 commented 5 months ago

No worries :grin: . Btw any timeline for the next release?

KillingSpark commented 5 months ago

I really wanna get my own PR resolved before that, it will increase performance by a few percent. I have a changeset locally that seems to do what I want. If this doesn't work out soon, I'll make a release. The PR I have there is purely internal changes so would be a minor version release, so not a big deal to push it back a bit.

JelteF commented 5 months ago

Author of derive_more here. The 1.0.0-beta.6 release is really stable (it fixes many things compared to than the 0.99.x branch).

khuey commented 5 months ago

It would be nice if you could cut a release with this PR in it. We're considering using ruzstd to provide support for zstd-compressed debug info in the backtrace crate, but the dependency on an old version of syn is a blocker to that.

JelteF commented 5 months ago

The 1.0.0-beta.6 release of derive_more depends on the 2.0 version of syn. I believe that should work for you.

JelteF commented 5 months ago

@khuey oh wait I misunderstood. You're asking for a release of zstd-rs not derive_more

khuey commented 5 months ago

@khuey oh wait I misunderstood. You're asking for a release of zstd-rs not derive_more

Yeah, that's correct.

I don't think we care whether this crate depends on syn 2 via derive-more 1.0 or whether it doesn't depend on syn at all but we don't want it to depend on syn 1.

KillingSpark commented 5 months ago

@khuey If I don't find the time to work further on #58 this week I'll cut a release on the weekend. So either way next week there will be a release without the syn 1.0 dependency

khuey commented 5 months ago

Great, thanks!