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

cmp: print verbose diffs as we find them #100

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Conversation

kov
Copy link

@kov kov commented Oct 5, 2024

I was thinking of ways to further improve the code and began looking at memory usage, as I thought collecting the differences for verbose output could be problematic for larger files. I managed to rework the code to not incur more overhead, and we actually gain a little bit of performance with the lower memory usage.

Before this change, we would first find all changes so we could obtain the largest offset we will report and use that to set up the padding.

Now we use the file sizes to estimate the largest possible offset. Not only does this allow us to print earlier, reduces memory usage, as we do not store diffs to report later, but it also fixes a case in which our output was different to GNU cmp's - because it also seems to estimate based on size.

Memory usage drops by a factor of 1000(!), without losing performance while comparing 2 binaries of hundreds of MBs:

Before:

Maximum resident set size (kbytes): 2489260

Benchmark 1: ../target/release/diffutils
cmp -l -b /usr/lib64/chromium-browser/chromium-browser /usr/lib64/firefox/libxul.so
Time (mean ± σ): 14.466 s ± 0.166 s [User: 12.367 s, System: 2.012 s]
Range (min … max): 14.350 s … 14.914 s 10 runs

After:

Maximum resident set size (kbytes): 2636

Benchmark 1: ../target/release/diffutils
cmp -l -b /usr/lib64/chromium-browser/chromium-browser /usr/lib64/firefox/libxul.so
Time (mean ± σ): 13.724 s ± 0.038 s [User: 12.263 s, System: 1.372 s]
Range (min … max): 13.667 s … 13.793 s 10 runs

Copy link

codecov bot commented Oct 5, 2024

Codecov Report

Attention: Patch coverage is 93.90244% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.11%. Comparing base (0bf04b4) to head (a316262).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/cmp.rs 93.82% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   84.95%   85.11%   +0.15%     
==========================================
  Files          12       12              
  Lines        5824     5818       -6     
  Branches      480      475       -5     
==========================================
+ Hits         4948     4952       +4     
+ Misses        856      846      -10     
  Partials       20       20              
Flag Coverage Δ
macos_latest 85.13% <93.90%> (+0.11%) ⬆️
ubuntu_latest 85.38% <93.90%> (+0.11%) ⬆️
windows_latest 22.99% <89.87%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kov kov force-pushed the cmp-mem branch 2 times, most recently from bca3d30 to ac94d7b Compare October 5, 2024 12:54
@oSoMoN oSoMoN self-requested a review October 5, 2024 17:25
Copy link
Collaborator

@oSoMoN oSoMoN left a comment

Choose a reason for hiding this comment

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

Very nice optimization! I have a couple of minor suggestions, but this otherwise looks good to me. Thanks!

src/cmp.rs Outdated Show resolved Hide resolved
src/cmp.rs Show resolved Hide resolved
Before this change, we would first find all changes so we could obtain
the largest offset we will report and use that to set up the padding.

Now we use the file sizes to estimate the largest possible offset.
Not only does this allow us to print earlier, reduces memory usage, as
we do not store diffs to report later, but it also fixes a case in
which our output was different to GNU cmp's - because it also seems
to estimate based on size.

Memory usage drops by a factor of 1000(!), without losing performance
while comparing 2 binaries of hundreds of MBs:

Before:

 Maximum resident set size (kbytes): 2489260

 Benchmark 1: ../target/release/diffutils \
 cmp -l -b /usr/lib64/chromium-browser/chromium-browser /usr/lib64/firefox/libxul.so
   Time (mean ± σ):     14.466 s ±  0.166 s    [User: 12.367 s, System: 2.012 s]
   Range (min … max):   14.350 s … 14.914 s    10 runs

After:

 Maximum resident set size (kbytes): 2636

 Benchmark 1: ../target/release/diffutils \
 cmp -l -b /usr/lib64/chromium-browser/chromium-browser /usr/lib64/firefox/libxul.so
   Time (mean ± σ):     13.724 s ±  0.038 s    [User: 12.263 s, System: 1.372 s]
   Range (min … max):   13.667 s … 13.793 s    10 runs
@oSoMoN oSoMoN merged commit 933230e into uutils:main Oct 8, 2024
27 checks passed
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.

2 participants