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.

Sign up for GitHub

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

Jump to bottom

xxhash: use ENV.O3 for performance #181691

Merged
merged 2 commits into from
Aug 21, 2024
Merged

xxhash: use ENV.O3 for performance #181691

merged 2 commits into from
Aug 21, 2024

Conversation

wen-long
Copy link
Contributor

big performance on mac m1
run xxhsum -b and compare

with -Os (homebrew default)

 1#XXH32                         :     102400 ->    71162 it/s ( 6949.4 MB/s)
 3#XXH64                         :     102400 ->   142409 it/s (13907.1 MB/s)
 5#XXH3_64b                      :     102400 ->   113125 it/s (11047.4 MB/s)
11#XXH128                        :     102400 ->   112679 it/s (11003.8 MB/s)

with -O3

 1#XXH32                         :     102400 ->    71310 it/s ( 6963.8 MB/s)
 3#XXH64                         :     102400 ->   142089 it/s (13875.9 MB/s)
 5#XXH3_64b                      :     102400 ->   365165 it/s (35660.6 MB/s)
11#XXH128                        :     102400 ->   364800 it/s (35625.0 MB/s)

that's 3x faster

and -O3 is default in https://github.com/Cyan4973/xxHash/blob/dbea33e47e7c0fe0b7c8592cd931c7430c1f130d/Makefile#L40

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@wen-long
xxhash: use ENV.O3 for performance
9410938 
big performance on mac m1
run xxhsum -b and compare

with -Os (homebrew default)
 1#XXH32                         :     102400 ->    71162 it/s ( 6949.4 MB/s)
 3#XXH64                         :     102400 ->   142409 it/s (13907.1 MB/s)
 5#XXH3_64b                      :     102400 ->   113125 it/s (11047.4 MB/s)
11#XXH128                        :     102400 ->   112679 it/s (11003.8 MB/s)

with -O3
 1#XXH32                         :     102400 ->    71310 it/s ( 6963.8 MB/s)
 3#XXH64                         :     102400 ->   142089 it/s (13875.9 MB/s)
 5#XXH3_64b                      :     102400 ->   365165 it/s (35660.6 MB/s)
11#XXH128                        :     102400 ->   364800 it/s (35625.0 MB/s)

that's 3x faster

and -O3 is default in https://github.com/Cyan4973/xxHash/blob/dbea33e47e7c0fe0b7c8592cd931c7430c1f130d/Makefile#L40
@github-actions GitHub Actions
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@carlocab carlocab added the CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. label Aug 20, 2024
carlocab
carlocab approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks! Would be good to verify that this doesn't break any dependents.

@wen-long
Copy link
Contributor Author

wen-long commented Aug 20, 2024 •
edited
Loading

have no idea why hashcat run into bug
I tried compile hashcat without homebrew
compile locally on my mac m1 macos 14.1.2, got random result, can not even predict the result before run it
even rename the hashcat directory name can also cause the fault

can you start a CI for hashcat but without this PR?
I build hashcat multiple times and the generated binary changed every time.

@wen-long
Copy link
Contributor Author

wen-long commented Aug 20, 2024 •
edited
Loading

I almost comfirm the fault is not relevant to compile but the directory name contains hashcat
to verity:

  1. build hashcat under a directory not named hashcat, it will run to fault
  2. rename(mv) the directory to ~/hashcat, run it again, will succeed

this is 05:37 AM in china, maybe I'm dreaming?

@carlocab
Copy link
Member

Thanks for investigating! I agree with your assessment that the dependent failure is not related.

@github-actions GitHub Actions
Copy link
Contributor

:shipit: @carlocab has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Aug 21, 2024
@BrewTestBot BrewTestBot enabled auto-merge August 21, 2024 03:18
@BrewTestBot BrewTestBot added this pull request to the merge queue Aug 21, 2024
Merged via the queue into Homebrew:master with commit 88bdf8c Aug 21, 2024
15 checks passed
@wen-long wen-long mentioned this pull request Aug 21, 2024
Closed
6 tasks
wen-long added a commit to wen-long/homebrew-core that referenced this pull request Aug 21, 2024
@wen-long
hashcat: fix failed to create metal library
2a49001 
this patch can precent macos hashcat v6.2.6 run into `failed to create metal library` and fix CI
the existing v6.2.6 bottle works fine, I dont know why, maybe it's caused by different command line tools or mac os version

Where did I find this patch code?

I build hashcat locally without homebrew, confirm the latest code works fine
And I do git bisect to find which commit fix the issue(screen shoot will attach)

```
git bisect start --term-old broken --term-new fixed  master v6.2.6
...
git bisect run ./b.sh
...
git bisect log
# fixed: [6716447dfce969ddde42a9abe0681500bee0df48] Add support for zero-length salts in Electrum $4 and $5
# broken: [e5b302363654cf4717dbe5bbe417504fc5d0ca72] hashcat 6.2.6
git bisect start '--term-old' 'broken' '--term-new' 'fixed' 'master' 'v6.2.6'
# fixed: [f3fe379571de944f6a9d457a12c1395572ad9f5d] Fix missing entry for -m 31800 in changes.txt
git bisect fixed f3fe379571de944f6a9d457a12c1395572ad9f5d
# fixed: [d19882ff71ad14cb2c252b47b1329bb39ad65fdd] Set a maximum thread count for -m 30901 to 32 for performance reasons
git bisect fixed d19882ff71ad14cb2c252b47b1329bb39ad65fdd
# broken: [29a3ea2583cfbf0dd0249fbb3916247e411709f1] Merge pull request Homebrew#3494 from ventaquil/feature/exodus-cleanup
git bisect broken 29a3ea2583cfbf0dd0249fbb3916247e411709f1
# fixed: [d008c5cb11c79af40da9132d3b62dc8f33c43e90] Merge pull request Homebrew#3522 from rjancewicz/rjancewicz/m07350-rakp-hmac-md5
git bisect fixed d008c5cb11c79af40da9132d3b62dc8f33c43e90
# fixed: [01938c374ce7b6fd4a8dfe3c522093eb42c2edd6] Merge remote-tracking branch 'origin/master' into bcrypt_sha256
git bisect fixed 01938c374ce7b6fd4a8dfe3c522093eb42c2edd6
# fixed: [66b22fa64472b4d809743c35fb05fc3c993a5cd2] Add support for Metal > 300 and reject support for older version
git bisect fixed 66b22fa64472b4d809743c35fb05fc3c993a5cd2
# broken: [6aa3e0882d83a6ee8f0a4decbdbad06b5be2affb] Mark some hash-modes for Apple Metal as unstable
git bisect broken 6aa3e0882d83a6ee8f0a4decbdbad06b5be2affb
# broken: [a15eeac44fb990879457ff9c1f8fa3ed22bd3765] Backport some AMD HIP headers from ROCm 5.3.0
git bisect broken a15eeac44fb990879457ff9c1f8fa3ed22bd3765
# broken: [f5c53a7e7736df04bc1b01e3ce6ffd06160f380e] added mode 30500
git bisect broken f5c53a7e7736df04bc1b01e3ce6ffd06160f380e
# broken: [adcd3e3e87a3448ecf8f3f82f0fdcc37d76eed81] Merge pull request Homebrew#3508 from piwvvo/mode-30500
git bisect broken adcd3e3e87a3448ecf8f3f82f0fdcc37d76eed81
# first fixed commit: [66b22fa64472b4d809743c35fb05fc3c993a5cd2] Add support for Metal > 300 and reject support for older version
```

b.sh
```
make distclean; make -j6
newdir=`mktemp -d`
echo $newdir
cp hashcat $newdir/
cp -r modules $newdir/
cp -r OpenCL $newdir/
cp hashcat.hcstat2 $newdir/
! $newdir/hashcat  --benchmark -m 0 -D 1,2 -w 2 -d 1
```

Note. move hashcat to a newdir evert time is necessary. as I mentioned in Homebrew#181691

And I try to find which line in 66b22fa cause the fix, finally, the macro __METAL__ is the key

the process is reproducible, only cost ~10 min
carlocab pushed a commit to wen-long/homebrew-core that referenced this pull request Aug 21, 2024
@wen-long @carlocab
hashcat: fix failed to create metal library
380eef5 
this patch can precent macos hashcat v6.2.6 run into `failed to create metal library` and fix CI
the existing v6.2.6 bottle works fine, I dont know why, maybe it's caused by different command line tools or mac os version

Where did I find this patch code?

I build hashcat locally without homebrew, confirm the latest code works fine
And I do git bisect to find which commit fix the issue(screen shoot will attach)

```
git bisect start --term-old broken --term-new fixed  master v6.2.6
...
git bisect run ./b.sh
...
git bisect log
# fixed: [6716447dfce969ddde42a9abe0681500bee0df48] Add support for zero-length salts in Electrum $4 and $5
# broken: [e5b302363654cf4717dbe5bbe417504fc5d0ca72] hashcat 6.2.6
git bisect start '--term-old' 'broken' '--term-new' 'fixed' 'master' 'v6.2.6'
# fixed: [f3fe379571de944f6a9d457a12c1395572ad9f5d] Fix missing entry for -m 31800 in changes.txt
git bisect fixed f3fe379571de944f6a9d457a12c1395572ad9f5d
# fixed: [d19882ff71ad14cb2c252b47b1329bb39ad65fdd] Set a maximum thread count for -m 30901 to 32 for performance reasons
git bisect fixed d19882ff71ad14cb2c252b47b1329bb39ad65fdd
# broken: [29a3ea2583cfbf0dd0249fbb3916247e411709f1] Merge pull request Homebrew#3494 from ventaquil/feature/exodus-cleanup
git bisect broken 29a3ea2583cfbf0dd0249fbb3916247e411709f1
# fixed: [d008c5cb11c79af40da9132d3b62dc8f33c43e90] Merge pull request Homebrew#3522 from rjancewicz/rjancewicz/m07350-rakp-hmac-md5
git bisect fixed d008c5cb11c79af40da9132d3b62dc8f33c43e90
# fixed: [01938c374ce7b6fd4a8dfe3c522093eb42c2edd6] Merge remote-tracking branch 'origin/master' into bcrypt_sha256
git bisect fixed 01938c374ce7b6fd4a8dfe3c522093eb42c2edd6
# fixed: [66b22fa64472b4d809743c35fb05fc3c993a5cd2] Add support for Metal > 300 and reject support for older version
git bisect fixed 66b22fa64472b4d809743c35fb05fc3c993a5cd2
# broken: [6aa3e0882d83a6ee8f0a4decbdbad06b5be2affb] Mark some hash-modes for Apple Metal as unstable
git bisect broken 6aa3e0882d83a6ee8f0a4decbdbad06b5be2affb
# broken: [a15eeac44fb990879457ff9c1f8fa3ed22bd3765] Backport some AMD HIP headers from ROCm 5.3.0
git bisect broken a15eeac44fb990879457ff9c1f8fa3ed22bd3765
# broken: [f5c53a7e7736df04bc1b01e3ce6ffd06160f380e] added mode 30500
git bisect broken f5c53a7e7736df04bc1b01e3ce6ffd06160f380e
# broken: [adcd3e3e87a3448ecf8f3f82f0fdcc37d76eed81] Merge pull request Homebrew#3508 from piwvvo/mode-30500
git bisect broken adcd3e3e87a3448ecf8f3f82f0fdcc37d76eed81
# first fixed commit: [66b22fa64472b4d809743c35fb05fc3c993a5cd2] Add support for Metal > 300 and reject support for older version
```

b.sh
```
make distclean; make -j6
newdir=`mktemp -d`
echo $newdir
cp hashcat $newdir/
cp -r modules $newdir/
cp -r OpenCL $newdir/
cp hashcat.hcstat2 $newdir/
! $newdir/hashcat  --benchmark -m 0 -D 1,2 -w 2 -d 1
```

Note. move hashcat to a newdir evert time is necessary. as I mentioned in Homebrew#181691

And I try to find which line in 66b22fa cause the fix, finally, the macro __METAL__ is the key

the process is reproducible, only cost ~10 min
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] approved these changes

@carlocab carlocab carlocab approved these changes

Assignees
No one assigned
Labels
CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wen-long @carlocab @BrewTestBot