valyala / gozstd

go wrapper for zstd
MIT License
434 stars 63 forks source link

Support setting a custom window size in Writer #24

Closed arl closed 4 years ago

arl commented 4 years ago

In #7 this adds support to custom window size (called WindowLog in the code, as in libzstd).

As discussed, setting this parameter is performed via a WriterParams structure in which letting all values to their default is equivalent to calling NewWriter.

I took the liberty to modify newWriterParams the non-public Writer constructor so that it now takes a WriterParam instead of having to add a new parameter to it. One example and 1 test have been added.

Fixes #7

codecov[bot] commented 4 years ago

Codecov Report

Merging #24 into master will increase coverage by 0.12%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   95.24%   95.37%   +0.12%     
==========================================
  Files           5        5              
  Lines         547      562      +15     
==========================================
+ Hits          521      536      +15     
  Misses         16       16              
  Partials       10       10              
Impacted Files Coverage Δ
writer.go 88.73% <100.00%> (+1.33%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2cfdaa6...56e2a38. Read the comment docs.

arl commented 4 years ago

I previously added a test TestWriterWindowLogGreaterThan27 but travis worker kills it 2 times over 3 because of increased memory.

arl commented 4 years ago

@valyala I believe I addressed your comments. There's an allocation in ResetWriterParams but not in ResetWriter. I see the coverage fails, I can add a test for ResetWriterParams

valyala commented 4 years ago

@valyala I believe I addressed your comments. There's an allocation in ResetWriterParams but not in ResetWriter.

It is unclear why ResetWriterParams allocates if params is passed by value into initCStream. Could you add a benchmark, which calls Reset and ResetWriterParams in a loop and see whether it really allocates by calling ReportAllocs() before the loop?

I see the coverage fails, I can add a test for ResetWriterParams

This would be nice to have.

arl commented 4 years ago

Indeed neither Reset nor ResetWriterParams allocate. I let the benchmark in writer_timing_test.go

valyala commented 4 years ago

@arl , thanks for the contribution!