-
Notifications
You must be signed in to change notification settings - Fork 88
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
destinations.ml: add a compressed_file_destination that uses zstandard #247
destinations.ml: add a compressed_file_destination that uses zstandard #247
Conversation
10d0a2f
to
cd311c3
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think this review got accidentally marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
83b8a37
to
246b494
Compare
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>
7ae4414
to
aa6935b
Compare
Signed-off-by: Hao Lian <hi@haolian.org>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Merged as part of 7ac635e. Thanks! |
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_filename
labeledargument. This is because, sometimes, magic-trace uses
/proc/self/fd/$FD
paths as a way to prevent a race condition whenserving traces.