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

klauspost/compress is 21 megabytes #220

Closed
nhooyr opened this issue Apr 2, 2020 · 9 comments · Fixed by #240
Closed

klauspost/compress is 21 megabytes #220

nhooyr opened this issue Apr 2, 2020 · 9 comments · Fixed by #240
Milestone

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Apr 2, 2020

Some user's reported an issue with the dependency being large diamondburned/arikawa#11

I confirmed, the dependency is 21 megabytes.

I think the best approach would be to vendor only the code we need from klauspost/compress.

cc @klauspost

@diamondburned
Copy link

Seeing how the compress PR used github.com/klauspost/compress/flate, can the stdlib compress/flate be used instead?

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 2, 2020

stdlib compress/flate is significantly slower. See klauspost/compress#216 (comment)

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 2, 2020

And allocates much more memory.

@diamondburned
Copy link

I think 2x slower isn't that big of a deal. If anything, the library can also allow an interface to give developers choices when it comes to performance-intensive tasks.

@diamondburned
Copy link

diamondburned commented Apr 3, 2020

Alternatively, I think @klauspost should really split up his repositories. Seeing how the largest file seems to be from compress/zstd, he could probably split 5 of his subpackages into 5 repositories, and we could probably import only the flate one.

image

Alternatively again, @klauspost should PR his things into the stdlib.

@ravener
Copy link

ravener commented Apr 3, 2020

The question is why is he pushing test data to the repo.

Anyway vendoring only needed code sounds good.

@klauspost
Copy link

No. If 21 megs is a problem for you, do what you want.

@nhooyr nhooyr added this to the v1.8.5 milestone Apr 13, 2020
@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 14, 2020

The question is why is he pushing test data to the repo.

Everyone pushes tests to repos.

Like @klauspost said, I'm not entirely sure this is a problem worth solving.

@nhooyr nhooyr modified the milestones: v1.8.5, v1.9.0 Apr 14, 2020
@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 15, 2020

I think the default should use stdlib and then I can maintain a separate branch if there's enough interest until/if stateless compression ends up in the stdlib. It's always nice to have zero dependencies as well.

@nhooyr nhooyr modified the milestones: v1.9.0, v1.8.6 Apr 15, 2020
@nhooyr nhooyr modified the milestones: v1.8.6, v1.8.7 May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
@nhooyr nhooyr closed this as completed Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants