Skip to content

uint256: minor improvement for Mul#167

Merged
holiman merged 1 commit intoholiman:masterfrom
AaronChen0:master
May 15, 2024
Merged

uint256: minor improvement for Mul#167
holiman merged 1 commit intoholiman:masterfrom
AaronChen0:master

Conversation

@AaronChen0
Copy link
Copy Markdown
Contributor

@AaronChen0 AaronChen0 commented May 14, 2024

Minor tweek of the code. Not sure where the boost comes from. But it does improve around 10% in my two x86-64 machines.

godbolt links:
old: https://go.godbolt.org/z/z8vqhG9r7
new: https://go.godbolt.org/z/M17Ycfzxx

Test

go test ./...

ok  	github.com/holiman/uint256	1.380s

Benchmark

goos: linux
goarch: amd64
pkg: github.com/holiman/uint256
cpu: AMD Ryzen 7 7735H with Radeon Graphics         
                      │     old     │                 new                 │
                      │   sec/op    │   sec/op     vs base                │
Mul/single/uint256-16   4.239n ± 2%   3.725n ± 1%  -12.14% (p=0.000 n=10)
Mul/single/big-16       37.05n ± 1%   36.96n ± 2%        ~ (p=0.403 n=10)
geomean                 12.53n        11.73n        -6.38%

goos: linux
goarch: amd64
pkg: github.com/holiman/uint256
cpu: Intel Core Processor (Broadwell, no TSX, IBRS)
                   │     old      │                new                 │
                   │    sec/op    │   sec/op     vs base               │
Mul/single/uint256   10.125n ± 4%   9.346n ± 2%  -7.69% (p=0.000 n=10)
Mul/single/big        79.24n ± 3%   79.11n ± 5%       ~ (p=0.853 n=10)
geomean               28.32n        27.19n       -4.00%

@codecov
Copy link
Copy Markdown

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5ecf78c) to head (3cbed10).

Additional details and impacted files 
@@            Coverage Diff            @@
##            master      #167   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1628      1626    -2     
=========================================
- Hits          1628      1626    -2

@holiman
Copy link
Copy Markdown
Owner

holiman commented May 14, 2024

Interesting!

name                  old time/op  new time/op  delta
Mul/single/uint256-8  15.1ns ± 0%  10.6ns ±35%  -29.68%  (p=0.000 n=8+10)

@chfast lgty?

@AaronChen0
Copy link
Copy Markdown
Contributor Author

AaronChen0 commented May 14, 2024 •
edited
Loading

New: https://go.godbolt.org/z/5E8TahK9j
Old: https://go.godbolt.org/z/nY31bsxdb

With Go 1.22.1, the new code has 7 less MOVQ instructions. Other instructions are the same amount.
Possibly, this is the reason.

chfast
chfast approved these changes May 14, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

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

LGTM as this only moves the values to different variables and changes the order of operations.

@chfast
Copy link
Copy Markdown
Collaborator

chfast commented May 14, 2024

New: https://go.godbolt.org/z/5E8TahK9j Old: https://go.godbolt.org/z/nY31bsxdb

With Go 1.22.1, the new code has 7 less MOVQ instructions. Other instructions are the same amount. Possibly, this is the reason.

godbolt has also the "diff view": https://go.godbolt.org/z/ddcK6TzKK

It looks the new code spills less values from registers to stack. Good change.

@holiman holiman merged commit 66a4528 into holiman:master May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chfast chfast chfast approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AaronChen0 @holiman @chfast