Skip to content

optimize sql.HashOf#2385

Merged
max-hoffman merged 1 commit intodolthub:mainfrom
reusee:main
Mar 12, 2024
Merged

optimize sql.HashOf#2385
max-hoffman merged 1 commit intodolthub:mainfrom
reusee:main

Conversation

@reusee
Copy link
Contributor

@reusee reusee commented Mar 9, 2024

  • pool *xxhash.Digest objects
  • use fmt.Fprintf to write to hash

benchmark stats

oos: linux
goarch: amd64
pkg: github.com/dolthub/go-mysql-server/sql
cpu: AMD Ryzen 9 7900 12-Core Processor
                  │     b1      │                 b2                  │
                  │   sec/op    │    sec/op     vs base               │
HashOf-24           79.65n ± 4%   70.86n ±  7%  -11.03% (p=0.002 n=6)
ParallelHashOf-24   10.47n ± 4%   11.85n ± 19%        ~ (p=0.368 n=6)
geomean             28.88n        28.98n         +0.32%

                  │     b1     │                   b2                   │
                  │    B/op    │    B/op     vs base                    │
HashOf-24           4.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
ParallelHashOf-24   4.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
geomean             4.000                    ?                      ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean

                  │     b1     │                   b2                   │
                  │ allocs/op  │ allocs/op   vs base                    │
HashOf-24           2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
ParallelHashOf-24   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
geomean             2.000                    ?                      ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean

@max-hoffman
Copy link
Contributor

The change seems fine, but I think the benchmarks might need to save the result to a global variable to avoid dead code elimination.

* pool *xxhash.Digest objects
* use fmt.Fprintf to write to hash

benchmark stats

oos: linux
goarch: amd64
pkg: github.com/dolthub/go-mysql-server/sql
cpu: AMD Ryzen 9 7900 12-Core Processor
                  │     b1      │                 b2                  │
                  │   sec/op    │    sec/op     vs base               │
HashOf-24           79.65n ± 4%   70.86n ±  7%  -11.03% (p=0.002 n=6)
ParallelHashOf-24   10.47n ± 4%   11.85n ± 19%        ~ (p=0.368 n=6)
geomean             28.88n        28.98n         +0.32%

                  │     b1     │                   b2                   │
                  │    B/op    │    B/op     vs base                    │
HashOf-24           4.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
ParallelHashOf-24   4.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
geomean             4.000                    ?                      ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean

                  │     b1     │                   b2                   │
                  │ allocs/op  │ allocs/op   vs base                    │
HashOf-24           2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
ParallelHashOf-24   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.002 n=6)
geomean             2.000                    ?                      ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean
@reusee
Copy link
Contributor Author

reusee commented Mar 12, 2024

The change seems fine, but I think the benchmarks might need to save the result to a global variable to avoid dead code elimination.

fixed. we're checking the error return value so the call will not be eliminated by the compiler.

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for taking the time to contribute

@max-hoffman max-hoffman merged commit 08b9011 into dolthub:main Mar 12, 2024
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