Skip to content

Improve performance by fusing kernels - #57

Open
au2001 wants to merge 11 commits into
1inch:masterfrom
codesensus-org:perf/fuse-kernels-and-align
Open

Improve performance by fusing kernels#57
au2001 wants to merge 11 commits into
1inch:masterfrom
codesensus-org:perf/fuse-kernels-and-align

Conversation

@au2001

@au2001 au2001 commented Jul 26, 2026

Copy link
Copy Markdown

Change Summary

What does this PR change?

It improves performance by 8-12% with these compounding changes:

  • aligning mp_number to 2x 16 bytes instead of 8x 4 bytes; and
  • merging kernels for iterating/transforming/scoring into a single one

(profanity_inverse is untouched in this PR, so there are still 2 launches total, down from 3-4)

Testing & Verification

How was this tested?

  • Unit tests
  • Integration tests
  • Manual testing (describe steps)
  • Verified on staging

I ran the --leading 0 scoring method on different GPUs with the base and head codebase.
For each resulting address, I ran an external verification script which found no mistake.
Here are the results for "70" variant of each NVIDIA generation:

GPU VRAM base head inverse-multiple gain
RTX 5070 12 GB 787 MH/s 876 MH/s -I 393216 +11.3%
RTX 4070 12 GB 659 MH/s 712 MH/s -I 393216 +8.0%
RTX 3070 8 GB 487 MH/s 536 MH/s -I 262144 +10.1%
RTX 2070 8 GB 328 MH/s 359 MH/s -I 16384 +9.4%
GTX 1070 8 GB 201 MH/s 225 MH/s -I 196608 +12.2%

Edit: Previous results used --benchmark, for which the compiler optimized the kernel by deleting the keccak + scoring. A patch has been added to prevent this from happening.

Notes:
During benchmarks, when reaching speeds of 2-3ms per round, the millisecond clock precision was insufficient.
I thus bumped it to microseconds for increased accuracy.

A bug also prevented from enabling profiling: clCreateCommandQueueWithProperties on OpenCL 2.0+ expects a zero terminated list of name/value pairs, not a bitfield like 1.2 – 0 just so happened to be interpreted as an empty list.

Risk Assessment

Risk Level:

  • Low - Minor changes, no operational impact
  • Medium - Moderate changes, limited impact, standard rollback available
  • High - Significant changes, potential operational impact, complex rollback

Risks & Impact

Fully backwards-compatible with no user-facing changes.
But changes how the kernels work in a non-trivial way.
Requires clearing the cache to see changes (and to rollback).

Disclaimer: this PR was assisted by Claude Opus 5 for code exploration & benchmark scripts.
However, all code being submitted has been hand-written – and I'm a human :)

@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown

Thanks for this — the fusion and the 16-byte alignment are both well-motivated, the commit history is easy to follow, and the two side fixes (queue properties, microsecond timing) are worth having on their own.

I checked that the refactor is behaviour-preserving: the doubles bit trick is equivalent for all 256 byte values, profanity_byte() matches the old uchar reinterpretation over random addresses, and the word-wise --exact compare matches the old byte-wise one over random masks built the way Mode::matching builds them. Dropping the pInverse address write is safe too, since profanity_inverse rewrites that whole range every round.

One blocker, then a few smaller asks.

Blocker: the inline helpers need to be static inline

OpenCL C follows C99, where inline without static or extern is an inline definition rather than an external one. If the compiler declines to inline a call, no body is emitted and the symbol is left undefined. That is what happens here with the clang front end that ROCm, PoCL, Intel NEO, Mesa Rusticl and Apple all use:

$ cat keccak.cl profanity.cl | clang -x cl -cl-std=CL1.2 -Xclang -finclude-default-header \
    -c -O2 -DPROFANITY_INVERSE_SIZE=255 -DPROFANITY_MAX_SCORE=40 -o /tmp/pr57.o -
$ nm -u /tmp/pr57.o | grep profanity
                 U profanity_iterate
                 U profanity_score_fn_mirror

Undefined symbols by optimisation level: -O0 → 10 (i.e. all ten helpers), -O1 → 1, -O2 → 2, -O3 → 4. master is clean at every level, so there is no optimisation setting where this is safe. NVIDIA's front end inlines unconditionally, which is why none of your five cards show it.

Adding static to all ten inline functions (profanity_iterate, profanity_byte, and the eight profanity_score_fn_*) removes every undefined symbol at every level, keeps the -cl-std=CL1.2 -Werror build clean, and costs about 7% of emitted .text.

Running tests/ under PoCL is a cheap way to confirm this, since tests/testutil.hpp builds the full keccak.cl + profanity.cl + harness.cl program.

The stale kernel cache is a hard failure, not just stale performance

getDeviceCacheFilename() only encodes inverse-size and device id, so anyone who has run profanity2 before will load a cached binary that has no profanity_iterate_score_* kernel. clCreateKernel then returns NULL and the program exits with failed to create kernel "profanity_iterate_score_leading" instead of running at the old speed. The PR description reads as if the only consequence were not seeing the speedup.

Could you add a README note like the one #56 added? Making the cache key a hash of the sources plus build options would fix this class of problem permanently, but that is a separate PR and not yours to carry.

Please validate on at least one non-NVIDIA device

All five data points are NVIDIA. Fusing raises the amount of live state in a single kernel, and because bContract is a runtime argument the second ethhash stays in the register budget even for plain address runs — occupancy effects there are vendor-specific. The README still carries an RX 480 row and four Apple Silicon rows, and profanity.cl itself warns that the RX 480 is sensitive to changes in this area. One AMD and one Apple Silicon run would be enough to be comfortable.

Smaller things

  • pInverse is read-only in the fused kernel now, so __global const mp_number * const may buy a little more on NVIDIA through the read-only path.
  • SpeedSample::sample still divides by delta with no zero guard, which yields inf in the displayed speed. Microseconds make that far less likely, but you are touching that line anyway.
  • The README "Current version" benchmark table was measured with the old four-launch --benchmark, which no longer means the same thing after the fusion — worth a note or a refresh.
  • Minor consistency point: the contract path reads bytes through a uchar* cast while everything else goes through profanity_byte().

Nothing else in the diff concerned me. The curve constants, the RLP prefixes, seed generation and the private-key derivation are all untouched, and the result buffer still carries only the address.

@galekseev

Copy link
Copy Markdown

Hi, @au2001 . Thanks for the PR.
The blocker for merging is that it breaks compatibility with non-NVIDIA devices.
It would be great if you could fix it.

@au2001

au2001 commented Jul 31, 2026

Copy link
Copy Markdown
Author

Hi @galekseev, sorry for missing that, should be fixed now

I refreshed the README benchmarks too, but wasn't able to get an RX 480 specifically, so I left it at the old value

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