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

Fix the missing parallel feature flag for ark-serialize #833

Closed
wants to merge 3 commits into from

Conversation

weikengchen
Copy link
Member

Description

This appears to be the blocker for a number of PRs since the latest test no longer allows a feature that is not declared in the Cargo.toml.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Re-reviewed Files changed in the GitHub PR explorer

N/A:

  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md

@weikengchen weikengchen requested review from a team as code owners May 29, 2024 05:51
@weikengchen weikengchen requested review from z-tech, Pratyush and mmagician and removed request for a team May 29, 2024 05:51
@weikengchen weikengchen added the P-high Priority: high label May 29, 2024
@weikengchen
Copy link
Member Author

Actually this is not as easy as I think: the problem is that we also implement Valid for things like Rc, but such structure is not "sendable".

@weikengchen weikengchen marked this pull request as draft May 29, 2024 05:59
@weikengchen
Copy link
Member Author

Should we comment out the current parallel implementation then? The problem seems to be as follows: parallel check always requires the elements to be sendable to other threads, and therefore this places a limitation

@Pratyush
Copy link
Member

This is a problem introduced by #811. I would rather that we revert the part there with removes Valid: Send instead of giving up on parallel check

@weikengchen weikengchen reopened this May 29, 2024
@weikengchen weikengchen marked this pull request as ready for review May 29, 2024 06:39
@weikengchen
Copy link
Member Author

I made an edit---now it has a different implementation.

Since all the batch_check requests would be reduced to the base primitives, and many trivial data structures may not benefit from parallel check:

  • now, the parallel check is not going to be applied to Valid in general
  • the base primitive needs to opt-in to accept parallel check
  • now, only EC points and pairing element are opted in

@weikengchen
Copy link
Member Author

weikengchen commented May 29, 2024

limitation of the current implementation: it copies. This is due to the argument being "Impl Iterator" and you cannot make it parallelizable without first pulling all the elements out from the iterator.

The good thing, however, is that for EC points and pairing, such copying is usually worthy.

Comment on lines +247 to +252
batch
.map(|x| (*x).clone())
.collect::<Vec<T>>()
.into_par_iter()
.try_for_each(|e| e.check())?;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does the par_bridge solution not work here?

Copy link
Member

Choose a reason for hiding this comment

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

ParallelBridge is implemented for Iterator + Send. We no longer have Send bound on batch here. So we'd need to put an explicit bound: batch: impl Iterator<Item = &'a T> + Send, but this propagates all the way up to Valid, and would in turn lead to two definitions of batch_check:

    #[cfg(feature = "parallel")]
    fn batch_check<'a>(batch: impl Iterator<Item = &'a Self> + ParallelBridge)...

    #[cfg(not(feature = "parallel"))]
    fn batch_check<'a>(batch: impl Iterator<Item = &'a Self>) ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants