Skip to content
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

pack: Add support for using a common pack dir for multiple traces #3735

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Keno
Copy link
Member

@Keno Keno commented Apr 24, 2024

When packaging traces from CI, it's fairly commong to have hundreds of traces that all basically share the exact same files. This can lead to some fairly large traces after packing. Of course, some file-systems support block-level deduplication and a compression library would certainly be able to dedup it back down as well, but it'd be faster to not create trace directories that big on disk in the first place.

This adds a --pack-dir command to rr pack <traces...>, which is used as a the common pack dir for all traces. Rather than packing files into their own trace dirs, they will be packed into the pack-dir, with relative symlinks from the original trace directories to the pack dir. An unmodified rr will be able to replay these as long as the pack dir is moved along with the trace dirs.

We've had this feature on the JuliaLang fork for a while, but it was tangled up with #2529 and I don't believe I ever PR'd it separately. I think there's a stronger case for this part of the feature, because it doesn't require any unpacking steps on the receiving side and is best done during the packing process.

@Keno Keno requested a review from rocallahan April 24, 2024 23:23
When packaging traces from CI, it's fairly commong to have hundreds of traces
that all basically share the exact same files. This can lead to some fairly
large traces after packing. Of course, some file-systems support block-level
deduplication and a compression library would certainly be able to dedup it
back down as well, but it'd be faster to not create trace directories that
big on disk in the first place.

This adds a `--pack-dir` command to `rr pack <traces...>`, which is used
as a the common pack dir for all traces. Rather than packing files into
their own trace dirs, they will be packed into the `pack-dir`, with relative
symlinks from the original trace directories to the pack dir. An unmodified
rr will be able to replay these as long as the pack dir is moved along
with the trace dirs.
Keno added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Apr 24, 2024
This bumps rr to current master, with rebased xcr0 patches.
It drops the `patch --index-dir` patch which was rejected upstream and we do not use (as far as I know). The `pack --pack-dir` patch is rebased and resubmitted upstream as rr-debugger/rr#3735.
maleadt pushed a commit to JuliaPackaging/Yggdrasil that referenced this pull request Apr 25, 2024
This bumps rr to current master, with rebased xcr0 patches.
It drops the `patch --index-dir` patch which was rejected upstream and we do not use (as far as I know). The `pack --pack-dir` patch is rebased and resubmitted upstream as rr-debugger/rr#3735.
Copy link
Collaborator

@rocallahan rocallahan left a comment

Choose a reason for hiding this comment

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

Generally looks good except for this <filesystem> issue.

@@ -13,6 +13,7 @@
#include <unistd.h>

#include <algorithm>
#include <filesystem>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will require gcc 9.1 and stop rr from building on Ubuntu 18 systems, which I believe works currently. Can we just reimplement filesystem::relative, or is that hard?

@@ -528,7 +536,8 @@ static map<string, string> compute_canonical_symlink_map(
* for all files with that hash.
*/
static map<string, string> compute_canonical_mmapped_files(
const string& trace_dir) {
const string& trace_dir,
PackDir &pack_dir) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

& before the space please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants