Skip to content

Internalize or remove Zlib implementation #2249

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

Closed
eerhardt opened this issue Jan 25, 2019 · 5 comments
Closed

Internalize or remove Zlib implementation #2249

eerhardt opened this issue Jan 25, 2019 · 5 comments
Labels
API Issues pertaining the friendly API
Milestone

Comments

@eerhardt
Copy link
Member

We have public Zlib types. We should at least make them internal, and remove them if possible.

public sealed class ZDeflateStream : Stream

@eerhardt eerhardt added the API Issues pertaining the friendly API label Jan 25, 2019
@eerhardt
Copy link
Member Author

@TomFinley - do you think we can remove this code altogether? I tried to find callers of it, but I couldn't see exactly who invokes it.

@Ivanidzo4ka
Copy link
Contributor

I can be wrong, but I think we decided to go with C# inflate/deflate algorithm, because our internal zlib is actually modified version of zlib code which is impossible to open source.
It should be safe to remove all zlib code and references from this repo.

@TomFinley
Copy link
Contributor

It's been some months since the evaluation, but when we did I believe we found that while compression of just raw zlib was considerably faster than what was in System.IO.Compression, decompression (which is far more important) was not that far off. And I think I opined at the time that if fast compression/decompression speeds was actually the primary goal we may as well use something more advanced than zlib anyway. So yes, from my perspective may as well remove. (But, this was a long time ago, and I think it was around the same time I performed an, um, practical demonstration of the phrase "bus number." :) So, I may not remember the circumstances perfectly.)

@danmoseley
Copy link
Member

System.Io .Compression these days is implemented around Zlib these days : corefx has a copy of it. One would hope it is possible to get performance close to directly calling zlib, but I don't know.

eerhardt added a commit to eerhardt/machinelearning that referenced this issue Jan 28, 2019
This code is not being called by ML.NET, and it was not intended to be public API. Removing.

Fix dotnet#2249
@TomFinley
Copy link
Contributor

x has a copy of it. One would hope it is possible to get performance close to directly calling zlib, but I don't know.

So I would have hoped too. Indeed, back when .NET Framework 4.5 was shipped, strange as it may seem the thing I was most looking forward to was that they were changing from their own internal implementation of DEFLATE, to actually using zlib. It was a definite improvement -- the pre-4.5 situation was just flat out awful -- but still to my disappointment considerably slower, by about 4x IIRC. Very disappointing. Now then, the situation now is a lot better, still a bit slower though at least on compression, at least, if I remember the evaluation we did earlier this year all right.

eerhardt added a commit that referenced this issue Jan 29, 2019
This code is not being called by ML.NET, and it was not intended to be public API. Removing.

Fix #2249
@shauheen shauheen added this to the 0119 milestone Jan 29, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

No branches or pull requests

5 participants