destinations.ml: add a compressed_file_destination that uses zstandard #247
Conversation
Lilydemi
force-pushed
the
compressed-file-destination
branch
from
10d0a2f
to
cd311c3
Compare
Lilydemi
force-pushed
the
compressed-file-destination
branch
from
15a6a67
to
8a1eb3a
Compare
vendor/tracing/zero/destinations.ml
Outdated
| let com_size = buffer_size * 2 in | ||
| let com_level = 5 in | ||
| let compressed_buf = Iobuf.create ~len:com_size in | ||
| let file = Core_unix.openfile ~mode:[ O_CREAT; O_TRUNC; O_RDWR ] filename in |
There was a problem hiding this comment.
| let file = Core_unix.openfile ~mode:[ O_CREAT; O_TRUNC; O_RDWR ] filename in | |
| let file = Core_unix.openfile ~mode:[ O_CREAT; O_TRUNC; O_CLOEXEC; O_RDWR ] filename in |
In case this ends up being used with a trace processor shell-based -serve.
There was a problem hiding this comment.
Sorry, I think this review got accidentally marked as resolved.
There was a problem hiding this comment.
Just added the O_CLOEXEC in direct_file_destination as well. Does that resolve this review?
|
This addresses #154 (just to link the two) |
|
@Xyene ready for rereview if you have free time! |
Lilydemi
force-pushed
the
compressed-file-destination
branch
from
83b8a37
to
246b494
Compare
hlian
force-pushed
the
compressed-file-destination
branch
from
246b494
to
7ae4414
Compare
Trace files can be large and hard to share. Using zstd to compress trace files allows users to decrease their file sizes, as demonstrated here: ``` $ du -sh trace.fxt trace.fxt.zst 472K trace.fxt 56K trace.fxt.zst ``` This is a preliminary attempt at adding a compressed file destination. We have some open questions that we want to raise in the PR. One thing we had to do is we had to make an `original_filename` labeled argument. This is because, sometimes, magic-trace uses `/proc/self/fd/$FD` paths as a way to prevent a race condition when serving traces. Signed-off-by: Lidya Demilew <si.demilew@gmail.com>
Signed-off-by: Lidya Demilew <si.demilew@gmail.com>
hlian
force-pushed
the
compressed-file-destination
branch
from
7ae4414
to
aa6935b
Compare
Signed-off-by: Hao Lian <hi@haolian.org>
hlian
force-pushed
the
compressed-file-destination
branch
from
a706045
to
734703e
Compare
| let file_destination ~filename () = direct_file_destination ~filename () | ||
| let compressed_file_destination ?(buffer_size = 64 * 1024) ~filename () = | ||
| let buf = Iobuf.create ~len:buffer_size in | ||
| let compressed_size = buffer_size * 2 in |
There was a problem hiding this comment.
Why not Zstandard.compression_output_size_bound, instead of compressed_size*2? One example where compressed_size*2 is not quite right is if the file is empty for some reason (the zstd'd file will not also be empty).
There was a problem hiding this comment.
Also, very minor side note: comments go above the line you're commenting on, not below.
| let file = Core_unix.openfile ~mode:[ O_CREAT; O_TRUNC; O_CLOEXEC; O_RDWR ] filename in | ||
| let written = ref 0 in | ||
| let compression_context = Zstandard.Compression_context.create () in | ||
| let flush () = |
There was a problem hiding this comment.
Does this compress the full trace file at once? Would it be easy to change this to stream the file out, instead? I worry that this may cause users with relatively large trace files to OOM for no good reason. In particular, those users are the most likely to care about compressing their trace files in the first place.
Trace files can be large and hard to share. Using zstd to compress trace
files allows users to decrease their file sizes, as demonstrated here:
This is a preliminary attempt at adding a compressed file destination.
We have some open questions that we want to raise in the PR.
One thing we had to do is we had to make an
original_filenamelabeledargument. This is because, sometimes, magic-trace uses
/proc/self/fd/$FDpaths as a way to prevent a race condition whenserving traces.