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

Replace serde by zerocopy #122

Open
daprilik opened this issue Aug 8, 2023 · 4 comments
Open

Replace serde by zerocopy #122

daprilik opened this issue Aug 8, 2023 · 4 comments

Comments

@daprilik
Copy link

daprilik commented Aug 8, 2023

nix and serde are both quite heavy dependencies, and bump compile times significantly.

This can be done in a backwards-compatible manner by making them default features, while allowing folks who aren't interested in those features to specify default-features = false.

Note that personally, I'm far more interested in making nix optional vs. serde (simply due to the fact that in my use-case, I already happen to be using serde), but ideally, both should get feature-gated.

@cecton
Copy link
Member

cecton commented Aug 9, 2023

I don't think I can make serde optional. gptman depends entirely on bincode which depends entirely on serde.

But nix should have been optional.

@daprilik
Copy link
Author

daprilik commented Aug 9, 2023

Ahh, I didn't realize that's what gptman was doing. Fair enough!

In that case, if I might offer another suggestion (once again without looking at gptman's implementation at all 😅): maybe it'd be worth switching over to something lighter like zerocopy instead of serde + bincode here? Assuming all you're using bincode for is repr(C) structure de/serialization.

@cecton
Copy link
Member

cecton commented Aug 14, 2023

Sorry for the delay!

Hmm yes that might be possible I guess. @bgilbert might have opinion on this?

In any case, this looks like quite some work and I don't currently have bandwidth for intensive changes like this. Now if you think it would make gptman awesome I'm really open to the idea and I will gladly give time to review it.

Also keep in mind that the crate gpt is catching up with gptman and it's very possible that gptman will become obsolete at some point in the future. In which case I will probably retire this crate.

@daprilik
Copy link
Author

Oh, neat! I didn't realize there was another crate being developed to work with GPT (with a lighter dep tree than gptman). I doubt I'll migrate my current gptman integration over to it (especially now that nix has become an optional dependency), but I'll def keep an eye on that project to see how it evolves moving forwards.

In true Typical Open Source User fashion, it's unlikely that I'll have the bandwidth to contribute back to gptman in the near future either 😅

In any case, I suppose we can leave this issue open for the time being, in case there are other folks who'd be interested in seeing these changes happen and/or would be inclined to make these sorts of changes themselves.

@cecton cecton changed the title Request to make nix and serde optional dependencies Replace serde by zerocopy Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants