Skip to content

BUG: fix FNV-1a 64-bit selection by using NPY_SIZEOF_UINTP#31035

Merged
seberg merged 3 commits intonumpy:mainfrom
math-hiyoko:fix_size_t
Mar 20, 2026
Merged

BUG: fix FNV-1a 64-bit selection by using NPY_SIZEOF_UINTP#31035
seberg merged 3 commits intonumpy:mainfrom
math-hiyoko:fix_size_t

Conversation

@math-hiyoko
Copy link
Copy Markdown
Contributor

@math-hiyoko math-hiyoko commented Mar 19, 2026

PR summary

close #31034

benchmark result
Change Before [621413b] After [b9ccff1] <fix_size_t> Ratio Benchmark (Parameter)
- 807±30μs 746±10μs 0.92 bench_lib.Unique.time_unique_values(200000, 90.0, 0.2, <class 'numpy.float64'>)
- 1.64±0.02ms 1.52±0.01ms 0.92 bench_lib.UniqueIntegers.time_unique_values(100000, 5000, <class 'numpy.int64'>)
- 1.26±0.01ms 1.16±0.02ms 0.92 bench_lib.UniqueIntegers.time_unique_values(100000, 5000, <class 'numpy.uint32'>)
- 14.8±0.1ms 13.6±0.1ms 0.92 bench_lib.UniqueIntegers.time_unique_values(1000000, 5000, <class 'numpy.int64'>)
- 3.38±0.05μs 3.10±0.03μs 0.92 bench_lib.UniqueIntegers.time_unique_values(200, 25, <class 'numpy.int64'>)
- 1.04±0.01ms 936±9μs 0.9 bench_lib.UniqueIntegers.time_unique_values(100000, 5000, <class 'numpy.int16'>)
- 9.87±0.07ms 8.75±0.07ms 0.89 bench_lib.UniqueIntegers.time_unique_values(1000000, 5000, <class 'numpy.int16'>)
- 11.8±0.1ms 10.5±0.09ms 0.89 bench_lib.UniqueIntegers.time_unique_values(1000000, 5000, <class 'numpy.uint32'>)
- 37.1±0.1ms 29.4±0.1ms 0.79 bench_lib.UniqueIntegers.time_unique_values(1000000, 250000, <class 'numpy.int64'>)
- 29.5±0.2ms 22.6±0.2ms 0.77 bench_lib.UniqueIntegers.time_unique_values(1000000, 250000, <class 'numpy.uint32'>)

seberg
seberg reviewed Mar 19, 2026
View reviewed changes
numpy/_core/src/multiarray/fnv.c Outdated
return (size_t)npy_fnv1a_64(buf, len, FNV1A_64_INIT);
#else /* NPY_SIZEOF_SIZE_T == 4 */
return (size_t)npy_fnv1a_32(buf, len, FNV1A_32_INIT);
#if NPY_SIZEOF_INTP == 8
Copy link
Copy Markdown
Member

@seberg seberg Mar 19, 2026

Choose a reason for hiding this comment

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

FWIW, I think it's fine to only change this and keep size_t throughout. (can add a comment that intp is the same size as size_t).
uintp feels a bit more natural, but it doesn't matter much. Some of the calculations below also seem unrelated, but I guess they also don't really matter (since they don't seem to generate new warnings).

(if this was C++, constexpr would be nice I guess, but dunno it's worth to change that.)

Anyway, this also looks completely fine, though.

Copy link
Copy Markdown
Contributor Author

@math-hiyoko math-hiyoko Mar 19, 2026

Choose a reason for hiding this comment

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

Thanks for the review.

I updated the condition to use NPY_SIZEOF_UINTP and reverted the unrelated changes.

@math-hiyoko math-hiyoko changed the title BUG: fix FNV-1a 64-bit selection by using NPY_SIZEOF_INTP BUG: fix FNV-1a 64-bit selection by using NPY_SIZEOF_UINTP Mar 20, 2026
@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Mar 20, 2026

I don't think the loongarch64 or ppc64le failures are real.

That said, I think maybe the cp312-macosx_286_64 failure is real, maybe?

delocate.libsana.DelocationError: Library dependencies do not satisfy target MacOS version 10.13:
  /private/var/folders/9n/h1d0pgxn4f5bbpg9mrrft9140000gn/T/tmpyq6zeudq/wheel/numpy/.dylibs/libunwind.1.0.dylib has a minimum target of 14.0
  Set the environment variable 'MACOSX_DEPLOYMENT_TARGET=14.0' to update minimum supported macOS for this wheel.

Or maybe you just got unlucky and this started breaking on your PR for some reason.

Can you try rebasing on current main to see if it goes away?

Sorry you're hitting issues in our CI. I think this change looks correct to me and great catch - nice to see such huge speedups!

@ngoldbaum ngoldbaum added the 09 - Backport-Candidate PRs tagged should be backported label Mar 20, 2026
@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Mar 20, 2026

Also maybe add a release note for the performance improvement? No need to if you don't want to though.

@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Mar 20, 2026

That said, I think maybe the cp312-macosx_286_64 failure is real, maybe?

nah, it's happening on other PRs. This is unrelated. Sorry our CI is so flaky!

@ngoldbaum ngoldbaum added this to the 2.4.4 release milestone Mar 20, 2026
@seberg
Copy link
Copy Markdown
Member

seberg commented Mar 20, 2026

Fun that we have SIZEOF_UINTP, but not for most types... but since we have it, LGTM, thanks.

@seberg seberg merged commit 86a4cc9 into numpy:main Mar 20, 2026
80 of 83 checks passed
charris pushed a commit to charris/numpy that referenced this pull request Mar 23, 2026
@math-hiyoko @charris
BUG: fix FNV-1a 64-bit selection by using NPY_SIZEOF_UINTP (numpy#31035)
ecf9d36
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Mar 23, 2026
charris added a commit that referenced this pull request Mar 23, 2026
@charris
Merge pull request #31052 from charris/backport-31035
b2b1bc1 
BUG: fix FNV-1a 64-bit selection by using NPY_SIZEOF_UINTP (#31035)
@charris charris mentioned this pull request Mar 24, 2026
Merged
@charris charris removed this from the 2.4.4 release milestone Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg left review comments

@ngoldbaum ngoldbaum ngoldbaum approved these changes

Assignees

No one assigned

Labels

00 - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BUG: 64-bit FNV-1a path is never used due to undefined NPY_SIZEOF_SIZE_T

4 participants

@math-hiyoko @ngoldbaum @seberg @charris