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

Feat - batch hashing with multi-threading #764

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

aviadingo
Copy link
Contributor

This PR adds multi-thread batch operations to the following hashes:
Blake2s
Blake3
Keccak256
Keccak512
SHA-3

this is enabled by default. not sure wether this affects merkle tree operations or not.

@aviadingo aviadingo changed the title Feat - batch hashing done in parallel using multi-threading Feat - batch hashing with multi-threading Feb 9, 2025
if (config.n_threads == 0) {
num_chunks = (std::thread::hardware_concurrency()) << 1; // Adjust based on the number of threads
} else {
num_chunks = static_cast<size_t>(config.n_threads);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need to compute min(std::thread::hardware_concurrency(), config.n_threads) to not use more thread than you have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to a different way of computation, similar to msm. actually more tasks than threads is allowed.

@@ -15,6 +15,7 @@ namespace icicle {
struct HashConfig {
icicleStreamHandle stream = nullptr; /**< Stream for asynchronous execution. Default is nullptr. */
uint64_t batch = 1; /**< Number of hashes to perform in parallel (independently). Default is 1. */
uint64_t n_threads = 0; /**< Number of threades to run in parallel for CPU. 0 means autoselcet. Default is 0. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this field is only relevant to CPU, but what does it mean for CUDA and other backends?
For this reason, we put it in config-extension in other APIs (e.g. MSM, NTT).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to the config extension

Copy link
Collaborator

@yshekel yshekel left a comment

Choose a reason for hiding this comment

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

overall looks good, left a few comments.
I think you also need to update Merkle-tree CPU to use a single thread, as it relies on the tree parallelism already. @mickeyasa correct me if wrong

Copy link
Contributor

@mickeyasa mickeyasa left a comment

Choose a reason for hiding this comment

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

few comments inside

@aviadingo aviadingo requested a review from yshekel February 10, 2025 22:08
@aviadingo aviadingo requested a review from mickeyasa February 10, 2025 22:08
Copy link
Contributor

@mickeyasa mickeyasa left a comment

Choose a reason for hiding this comment

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

great

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.

3 participants