Opened 6 years ago

Closed 5 years ago

#11787 closed enhancement (fixed)

Can we save huge amounts of directory bandwith with Z_NO_FLUSH?

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, zlib, 025-backport, 026-triaged-0 026-triaged-1
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

It seems our default argument when compressing with zlib is Z_SYNC_FLUSH. That's not ideal, though: in theory we would get better compression by using Z_NO_FLUSH. But, how much better? And would that work with our code, or would we need other changes?

We should at least investigate this in 0.2.5, though if the fix is nontrivial, it can wait for 0.2.6.

Child Tickets

Attachments (1)

zlibtest.py (1.6 KB) - added by nickm 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by nickm

Status: newneeds_review

Totally untested patch in branch "bug11787" in my public repository

comment:2 Changed 6 years ago by nickm

I still don't know if the patch works. But if it does, I expect it to save something 7% for server descriptors, and who-knows-how-much for microdescs. (This is based on downloading all.z from turtles, uncompressing it, and recompressing it with gzip -9)

comment:3 Changed 6 years ago by nickm

(The actual number of bytes saved will depend on the number of compressed objects and their actual formats)

Changed 6 years ago by nickm

Attachment: zlibtest.py added

comment:4 Changed 6 years ago by nickm

I've attached a fun script to try compressing different numbers of descriptors and microdescriptors with and without Z_SYNC_FLUSH.

Preliminary results are that we should expect to save 5-7% when sending 10-40 server descriptors, and between 12-17% when sending 10-100 microdescriptors. Worth doing IMO.

comment:5 Changed 6 years ago by nickm

(This fix probably needs the #11648 fix as well, or i would expect it to sometimes cause us to truncate streams and omit many descriptors)

comment:6 in reply to:  1 Changed 6 years ago by wfn

Replying to nickm:

Totally untested patch in branch "bug11787" in my public repository

Not sure and maybe the patch is only for benchmarking / testing out things, but wouldn't the decompression part in torgzip.c also need changing?

302     switch (inflate(stream, complete_only ? Z_FINISH : Z_SYNC_FLUSH))

and

477     err = inflate(&state->stream, finish ? Z_FINISH : Z_SYNC_FLUSH);

or am I mistaken?

comment:7 Changed 6 years ago by nickm

I think Z_SYNC_FLUSH is safe on inflate. After all, there is only one correct output for inflating any compressed zlib stream. (Anybody want to look at the zlib source to see what's up?)

comment:8 Changed 6 years ago by nickm

Keywords: 025-backport added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

Moving to 0.2.6 with possible backport, since the effects of breaking this code would not be fun to try to solve late in a release series.

comment:9 Changed 6 years ago by nickm

Keywords: 026-triaged added

comment:10 Changed 6 years ago by nickm

Keywords: 026-triaged-0 added; 026-triaged removed

comment:11 Changed 6 years ago by nickm

Keywords: 026-triaged-1 added

comment:12 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.5.x-final

Merged to master, marking for possible backport. Seems to work ok in testing.

comment:13 Changed 5 years ago by arma

On first glance, this looks like exactly the sort of feature that doesn't need a backport. (Let's make 0.2.6 the new stable and leave 0.2.5 behind.)

comment:14 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Resolution: fixed
Status: needs_reviewclosed
Type: defectenhancement

ack

Note: See TracTickets for help on using tickets.