Add algorithm specification of XXH3 #750

Merged
adrien1018 merged 5 commits from dev into dev 2023-06-29 06:40:07 -05:00
adrien1018 commented 2022-10-24 07:07:22 -05:00 (Migrated from github.com)

Update xxhash_spec.md to include XXH3 specification. Closes #589 and #675.

Update `xxhash_spec.md` to include XXH3 specification. Closes #589 and #675.
Cyan4973 commented 2022-11-09 20:05:47 -06:00 (Migrated from github.com)

Thanks @adrien1018 ! This is great work !

I think this documentation is in good shape, just give me a bit more time to read it in detail.

Thanks @adrien1018 ! This is great work ! I think this documentation is in good shape, just give me a bit more time to read it in detail.
Cyan4973 (Migrated from github.com) reviewed 2023-06-28 01:58:20 -05:00
@ -52,2 +59,4 @@
- `X xor Y`: denotes the bit-wise XOR of `X` and `Y` (same width).
- `X | Y`: denotes the bit-wise OR of `X` and `Y` (same width).
- `~X`: denotes the bit-wise negation of `X`.
Cyan4973 (Migrated from github.com) commented 2023-06-28 01:58:19 -05:00

minor nit: use_ 64-bit modular (plural)

minor nit: use_ 64-bit modular (plural)
Cyan4973 (Migrated from github.com) reviewed 2023-06-28 02:00:44 -05:00
Cyan4973 (Migrated from github.com) commented 2023-06-28 02:00:44 -05:00

👍
It's interesting, the width of the modular arithmetic may be not obvious, making * alone possibly insufficient to describe the operation.

👍 It's interesting, the width of the modular arithmetic may be not obvious, making `*` alone possibly insufficient to describe the operation.
Cyan4973 (Migrated from github.com) reviewed 2023-06-28 02:45:14 -05:00
@ -327,0 +401,4 @@
To make sure that all input bits have a chance to impact any bit in the output digest (avalanche effect), the final step of the XXH3 algorithm is usually one of the two fixed operations that mix the bits in a 64-bit value. These operation are denoted `avalanche()` and `avalanche_XXH64()` in the following XXH3 description.
```c
Cyan4973 (Migrated from github.com) commented 2023-06-28 02:45:13 -05:00

kudos for giving this constant a name

kudos for giving this constant a name
Cyan4973 (Migrated from github.com) reviewed 2023-06-28 03:40:22 -05:00
@ -327,0 +622,4 @@
#### 129-240 bytes of input
The input is split into 16-byte (XXH3-64) or 32-byte (XXH3-128) chunks. The first 128 bytes are first mixed chunk by chunk, followed by an intermediate avalanche operation. Then the remaining full chunks are processed, and finally the last 16/32 bytes are treated as a chunk to process.
Cyan4973 (Migrated from github.com) commented 2023-06-28 03:40:21 -05:00

minor: this interface likely miss the secret pointer parameter,
the following code would only make sense if secret was a global variable only.

minor: this interface likely miss the `secret` pointer parameter, the following code would only make sense if `secret` was a global variable only.
Cyan4973 (Migrated from github.com) reviewed 2023-06-28 03:54:55 -05:00
@ -327,0 +730,4 @@
acc[i] = acc[i] xor secretWords[i];
acc[i] = acc[i] * PRIME32_1;
}
```
Cyan4973 (Migrated from github.com) commented 2023-06-28 03:54:55 -05:00

👍

👍
Cyan4973 (Migrated from github.com) approved these changes 2023-06-29 04:31:13 -05:00
Cyan4973 (Migrated from github.com) left a comment

This is great work.
I only noticed a couple of nits, don't worry I'll fix them directly in my branch.
I'm even likely to update the source in a couple of ways to better follows the text of this specification.

Thanks @adrien1018 !

This is great work. I only noticed a couple of nits, don't worry I'll fix them directly in my branch. I'm even likely to update the source in a couple of ways to better follows the text of this specification. Thanks @adrien1018 !
Cyan4973 (Migrated from github.com) reviewed 2023-06-29 06:44:04 -05:00
Cyan4973 (Migrated from github.com) commented 2023-06-29 06:44:03 -05:00

After reviewing the rest of the specification, I believe that the propose notification for 128-bit modular arithmetic does the job correctly.

After reviewing the rest of the specification, I believe that the propose notification for 128-bit modular arithmetic does the job correctly.
Cyan4973 (Migrated from github.com) reviewed 2023-06-29 07:20:54 -05:00
@ -327,0 +622,4 @@
#### 129-240 bytes of input
The input is split into 16-byte (XXH3-64) or 32-byte (XXH3-128) chunks. The first 128 bytes are first mixed chunk by chunk, followed by an intermediate avalanche operation. Then the remaining full chunks are processed, and finally the last 16/32 bytes are treated as a chunk to process.
Cyan4973 (Migrated from github.com) commented 2023-06-29 07:20:54 -05:00

I realize that not providing secret as an input parameter is a good way to improve brevity, which is beneficial for readability.
I think I'll keep it this way,
I just want to make it clear that secret is not necessarily the standard defaultSecret, it can also be any blob of bytes set by the user.
There's already a paragraph about this point, which is probably clear enough.
Let's just ensure it.

I realize that not providing `secret` as an input parameter is a good way to improve brevity, which is beneficial for readability. I think I'll keep it this way, I just want to make it clear that `secret` is not _necessarily_ the _standard_ `defaultSecret`, it can also be any blob of bytes set by the user. There's already a paragraph about this point, which is probably clear enough. Let's just ensure it.
Sign in to join this conversation.
No description provided.