Skip to content

zstd: If first block and 'final', encode direct. #251

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 27, 2020
Merged

zstd: If first block and 'final', encode direct. #251

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c432a5d
zstd: If first block and 'final', encode direct.
klauspost Mar 27, 2020
56a69db
Keep track of full written frames.
klauspost Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions zstd/encoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,18 @@ type encoder interface {
}

type encoderState struct {
w io.Writer
filling []byte
current []byte
previous []byte
encoder encoder
writing *blockEnc
err error
writeErr error
nWritten int64
headerWritten bool
eofWritten bool
w io.Writer
filling []byte
current []byte
previous []byte
encoder encoder
writing *blockEnc
err error
writeErr error
nWritten int64
headerWritten bool
eofWritten bool
fullFrameWritten bool

// This waitgroup indicates an encode is running.
wg sync.WaitGroup
Expand Down Expand Up @@ -114,6 +115,7 @@ func (e *Encoder) Reset(w io.Writer) {
s.encoder.Reset()
s.headerWritten = false
s.eofWritten = false
s.fullFrameWritten = false
s.w = w
s.err = nil
s.nWritten = 0
Expand Down Expand Up @@ -172,6 +174,22 @@ func (e *Encoder) nextBlock(final bool) error {
return fmt.Errorf("block > maxStoreBlockSize")
}
if !s.headerWritten {
// If we have a single block encode, do a sync compression.
if final && len(s.filling) > 0 {
s.current = e.EncodeAll(s.filling, s.current[:0])
var n2 int
n2, s.err = s.w.Write(s.current)
if s.err != nil {
return s.err
}
s.nWritten += int64(n2)
s.current = s.current[:0]
s.filling = s.filling[:0]
s.headerWritten = true
s.fullFrameWritten = true
return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not. This just hides the problem if I read this correctly.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not hiding any problems, but handles small < 1 block streaming encodes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand. The benchmark will be faster until some threshold and then it will degrade to previous levels. So from my POV it does not change much and just gives false hopes :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you are doing multiple blocks you will get the benefit of the concurrency. There will off course be a small dropoff as you get just above 1 block, but overall speed will be faster.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when we have enough for 1 block we don't know how much more will come, so we start compressing the first block.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but overall speed will be faster

Just want to repeat here as well that in case of HTTP server I don't want code to be faster by using more threads and CPU - those threads and CPU can be used to serve other requests/users. Otherwise I just make 1 request faster by delaying all the other requests.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I respectfully disagree, but will also repeat that this will likely be the case when full parallel compression.

First of all Go has very advanced preemption and handles this nicely.

Secondly you are just postponing the problem. If you have a 2 CPU system, the problem will occur as soon as you have 3 users and so on.

If you just make compression slower by using 1 of 2 cores, it will take (in theory) 2x as long, meaning you are strained for resources 2x as long instead of just getting it done faster AND give a better user experience when you are not limited for resources.

Assuming things scale linearly, you only can do xMB/s on your system and there is no reason to slow down every transaction, since the system is likely to behave the same when you reach that total limit anyway, ie. splitting the resources between the users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that parallel version has some significant overhead. So instead of 100% CPU it uses 130% CPU and the 2nd user now has only 70% of the 2nd core.

What is even worse is that parallel version is slower on small payloads - not faster. I even tried to compress ~300mb binary and got this result:

BenchmarkCZstd-12      	       1	1758534769 ns/op	 160.09 MB/s	         0.239 compression	1288323136 B/op	      38 allocs/op
BenchmarkGoZstd-12     	       1	3465210251 ns/op	  81.24 MB/s	         0.244 compression	1339945080 B/op	     680 allocs/op
BenchmarkGoZstd2-12    	       1	2187101331 ns/op	 128.72 MB/s	         0.244 compression	757003880 B/op	     186 allocs/op
BenchmarkS2-12         	       2	 746165990 ns/op	 377.29 MB/s	         0.363 compression	1275335332 B/op	    1597 allocs/op

EncodeAll version is still significantly faster...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must be doing something strange. Here is a bench that doesn't allocate (as much): https://gist.github.com/klauspost/446e9ab7aeae0b75d7974339b65df815

λ go test -bench=. -benchtime=10s
BenchmarkCZstd-32          13430            889105 ns/op         253.44 MB/s          25 B/op          0 allocs/op
BenchmarkGoZstd-32          7183           1545090 ns/op         145.84 MB/s      251130 B/op          1 allocs/op
BenchmarkGoZstd2-32         5437           2187727 ns/op         103.00 MB/s      112531 B/op          0 allocs/op
BenchmarkS2-32             22752            528732 ns/op         426.19 MB/s         975 B/op          8 allocs/op


var tmp [maxHeaderSize]byte
fh := frameHeader{
ContentSize: 0,
Expand Down Expand Up @@ -359,6 +377,9 @@ func (e *Encoder) Close() error {
if err != nil {
return err
}
if e.state.fullFrameWritten {
return s.err
}
s.wg.Wait()
s.wWg.Wait()

Expand Down