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

benchmark mode specifies nb of threads with -v #4235

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Jan 12, 2025

Now that the benchmark mode uses the same default nb of threads as normal CLI compression, it's no longer obvious how many threads are being used in benchmark. This can lead to incorrect conclusions, when comparing the benchmark result with earlier versions, which always use 1 thread by default.

With this PR, this information is displayed if the benchmark mode -b is combined with verbose output -v.

Example :

zstd -bv
Benchmarking at level 3 using 2 threads
 3#Lorem ipsum       :  10000000 ->   2983345 (x3.352),  205.2 MB/s,  866.3 MB/s

./zstd -b1e3v -S dirCalgary/b*    
Benchmarking 3 files from level 1 to 3 using 2 threads
 1#bib               :    111261 ->     40381 (x2.755),  266.2 MB/s, 1032.0 MB/s
 2#bib               :    111261 ->     39081 (x2.847),  243.3 MB/s   861.6 MB/s
 3#bib               :    111261 ->     37034 (x3.004),  196.4 MB/s   929.7 MB/s
 1#book1             :    768771 ->    349758 (x2.198),  161.5 MB/s, 1067.2 MB/s
 2#book1             :    768771 ->    317887 (x2.418),  139.5 MB/s,  824.2 MB/s
 3#book1             :    768771 ->    305288 (x2.518),  116.3 MB/s,  745.0 MB/s
 1#book2             :    610856 ->    228144 (x2.678),  238.9 MB/s, 1019.8 MB/s
 2#book2             :    610856 ->    215190 (x2.839),  182.4 MB/s,  860.3 MB/s
 3#book2             :    610856 ->    203784 (x2.998),  150.9 MB/s,  849.8 MB/s

compared to dev :

zstd -bv                                   
 3#Lorem ipsum       :  10000000 ->   2983345 (x3.352),  201.4 MB/s,  866.1 MB/s

zstd -b1e3v -S dirCalgary/b* 
Benchmarking levels from 1 to 3
Benchmarking /home/cyan/dev/bench/dirCalgary/bib 
 1#bib               :    111261 ->     40381 (x2.755),  267.1 MB/s, 1029.1 MB/s
 2#bib               :    111261 ->     39081 (x2.847),  245.2 MB/s   863.5 MB/s
 3#bib               :    111261 ->     37034 (x3.004),  194.7 MB/s   925.5 MB/s
Benchmarking /home/cyan/dev/bench/dirCalgary/book1 
 1#book1             :    768771 ->    349758 (x2.198),  204.3 MB/s, 1057.3 MB/s
 2#book1             :    768771 ->    317887 (x2.418),  133.5 MB/s,  822.4 MB/s
 3#book1             :    768771 ->    305288 (x2.518),  113.9 MB/s,  743.3 MB/s
Benchmarking /home/cyan/dev/bench/dirCalgary/book2 
 1#book2             :    610856 ->    228144 (x2.678),  229.4 MB/s, 1025.0 MB/s
 2#book2             :    610856 ->    215190 (x2.839),  182.2 MB/s,  859.0 MB/s
 3#book2             :    610856 ->    203784 (x2.998),  152.8 MB/s,  854.1 MB/s

@Cyan4973 Cyan4973 marked this pull request as ready for review January 12, 2025 11:05
@Cyan4973 Cyan4973 self-assigned this Jan 12, 2025
@terrelln
Copy link
Contributor

Now that the benchmark mode uses the same default nb of threads as normal CLI compression, it's no longer obvious how many threads are being used in benchmark

Is this the desired behavior? I'd expect that benchmark mode would use single threaded compression by default.

@Cyan4973
Copy link
Contributor Author

Is this the desired behavior? I'd expect that benchmark mode would use single threaded compression by default.

We discussed this behavior with @daniellerozenblit when she introduced MT by default in zstd,
and the consensus was that it made more sense for the benchmark to feature the same default as CLI, because casual users will generally expect that they are measuring the same thing.

That's definitely something that can still be discussed if you wish to.

In the meantime, this PR just displays this information to make sure users understand what is being measured, so the functionality remains valid whatever the decision is regarding default threads behavior.

@Cyan4973 Cyan4973 merged commit 319dc29 into dev Jan 16, 2025
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants