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.

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

Add additional space to store 64-bit hash codes on 32-bit machines #3359

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

d-torrance
Copy link
Member

This is a followup to #3335, checking off the final checkbox from the TODO list in #3335 (comment).

On 32-bit machines, thread local variables are stored in arrays of 32-bit chunks of memory. But now that we use 64-bit hash codes, we were getting the hash counter by concatenating two adjacent elements of the array, giving us unexpected results.

We add a new thread local variable to store these extra 32 bits, fixing this issue. This will be unnecessary if we switch to an atomic hash counter (see #3358).

Now that we can actually see what the expected result should be, we also update the hash code test for RR objects for 32-bit machines.

@d-torrance d-torrance changed the title 32 bit hash codes Add additional space to store 64-bit hash codes on 32-bit machines Jul 14, 2024
@mahrud
Copy link
Member

mahrud commented Jul 27, 2024

Wait so you're saying we earmark 32-bits extra on the stack so that when we read 64 bits from the hash counter we don't get junk? I'm concerned that we're reading a 64-bit integer on 32-bit systems! Why not make threadlocal variables in interpreter all 64 bits?

Also, is the problem with using size_t that hashes will be different on 32-bit vs 64-bit systems? Maybe we should make a hash_t typedef so this is clearer? ignore this, I just remembered that you did this already.

Tangentially, are there users out there with 32-bit systems who use the most recent M2 versions? (i.e. not users still running M2 v1.9 or something)

@d-torrance
Copy link
Member Author

Wait so you're saying we earmark 32-bits extra on the stack so that when we read 64 bits from the hash counter we don't get junk?

Yes, exactly.

I'm concerned that we're reading a 64-bit integer on 32-bit systems! Why not make threadlocal variables in interpreter all 64 bits?

That might be a better solution, but I think all of the other thread local variables are 32-bit ints.

Also, is the problem with using size_t that hashes will be different on 32-bit vs 64-bit systems? Maybe we should make a hash_t typedef so this is clearer?

hash_t is already a typedef for uint64_t.

Tangentially, are there users out there with 32-bit systems who use the most recent M2 versions? (i.e. not users still running M2 v1.9 or something)

Probably not. I occasionally run it on my 32-bit raspberry pi for giggles. I need it to build on several 32-bit architectures for Debian, though.

@mahrud
Copy link
Member

mahrud commented Jul 27, 2024

That might be a better solution, but I think all of the other thread local variables are 32-bit ints.

I could be wrong, but I think they're just ints, so which means somewhere down the line we might run into a situation involving thread local variables where the result (or at least a hash) is different between a 32-bit and 64-bit machine.

I need it to build on several 32-bit architectures for Debian, though.

Does Debian require all packages to build on 32-bit archs? I've always been a bit concerned about tests or examples that were removed because they failed on rare architectures.

@d-torrance
Copy link
Member Author

I could be wrong, but I think they're just ints, so which means somewhere down the line we might run into a situation involving thread local variables where the result (or at least a hash) is different between a 32-bit and 64-bit machine.

Theoretically, an int could be as little as 16 bits, but in practice, it's 32 bits on all architectures that anyone actually uses. (It's long that's the annoying one that varies.)

Does Debian require all packages to build on 32-bit archs? I've always been a bit concerned about tests or examples that were removed because they failed on rare architectures.

Kind of. There are three officially supported 32-bit architectures (i386, armhf, and armel), plus a bunch of non-official ones. If Macaulay2 stops building on any of the three official ones, then it won't migrate from unstable to testing on any of the architectures, 32-bit or 64-bit. There are ways around this if we ever decide to completely remove 32-bit support one day, though.


Converting this to a draft. I like the idea of making the array of thread-local variables hold 64 bits per slot instead of 32.

@d-torrance d-torrance marked this pull request as draft July 28, 2024 01:50
@d-torrance
Copy link
Member Author

I've looked at this a little more, and I'm not sure how to change the number of bits per element of the array without some more serious study of how pthreads work. For now, I'm proposing that we keep this hacky solution so that hashes work properly on 32-bit systems. I'll add a TODO comment.

On 32-bit machines, thread local variables are stored in arrays of
32-bit chunks of memory.  But now that we use 64-bit hash codes, we
were getting the hash counter by concatenating two adjacent elements
of the array, giving us unexpected results.

We add a new thread local variable to store these extra 32 bits,
fixing this issue.
Also fix the comments -- these hash codes differ based on 32-bit
v. 64-bit, not endianness.
@d-torrance d-torrance marked this pull request as ready for review July 30, 2024 13:10
Copy link
Member

@mahrud mahrud left a comment

Choose a reason for hiding this comment

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

Sounds good! I'll merge once tests past.

@mahrud mahrud merged commit 9901142 into Macaulay2:development Aug 2, 2024
6 checks passed
@d-torrance d-torrance deleted the 32-bit-hash-codes branch September 3, 2024 13:03
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