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.

Sign up for GitHub

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

Jump to bottom

take span instead of std::vector to simplify interface #190

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

take span instead of std::vector to simplify interface #190

wants to merge 1 commit into from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Mar 23, 2021 •
edited

modernize the interface a bit to take span rather than std::vector. For now, use our own span type, but this could use GSL or std::span in the future. The main motivation for this is to allow passing ranges of values without having to first copy them into a std::vector. It also simplifies some of the functions that currently take both a span and a std::vector. By making span be implicitly constructed from a vector, we can remove one of the overloads.

arvidn
arvidn commented Mar 23, 2021
View reviewed changes
src/schemes.cpp Outdated Show resolved Hide resolved
arvidn
arvidn commented Mar 23, 2021
View reviewed changes
src/util.hpp Outdated
};

using Bytes = span<uint8_t const>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bytes still mean the same thing even though it's an alias to span now

arvidn
arvidn commented Mar 23, 2021
View reviewed changes
src/util.hpp
explicit Bytes(const std::vector<uint8_t>& vecBytes)
: pData(vecBytes.data()), nSize(vecBytes.size())

span(const std::vector<typename std::remove_const<T>::type>& cont)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note how explicit was removed here. This allows implicit conversion from a vector to a span. This lets you pass either a vector or a pointer+size to an interface that just wants a sequence of objects.

xdustinface
xdustinface reviewed Mar 24, 2021
View reviewed changes
Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

I like the idea with span instead of vector for the parameters but shouldn't this be done for all API calls then? Right now it would introduce a mix of both and imo it makes sense to have a consistent interface.

Also see some thoughts below

src/schemes.cpp Outdated Show resolved Hide resolved
src/schemes.cpp Outdated Show resolved Hide resolved
src/schemes.cpp Outdated Show resolved Hide resolved
@arvidn arvidn marked this pull request as draft March 24, 2021 16:30
@arvidn
Copy link
Contributor Author

arvidn commented Mar 24, 2021

I marked this as a draft for now. I think I may pivot this into just doing the span change (more pervasively) and make another attempt at avoiding copying all the messages in a separate PR

@arvidn arvidn force-pushed the less-copying branch 2 times, most recently from f551c57 to e65d0a1 Compare March 26, 2021 14:09
arvidn
arvidn commented Mar 26, 2021
View reviewed changes
src/elements.cpp
@@ -64,7 +64,7 @@ G1Element G1Element::FromBytes(const Bytes& bytes)

G1Element G1Element::FromByteVector(const std::vector<uint8_t>& bytevec)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the FromByteVector() functions don't really serve a purpose with this patch anymore, since the Bytes overload can be used for std::vector as well. Since it's part of the public API though, I'm hesitant to remove it.

@arvidn arvidn changed the title avoid copying of public keys, messages in batch validation take span instead of std::vector to simplify interface Mar 26, 2021
@arvidn arvidn marked this pull request as ready for review March 31, 2021 06:37
@arvidn arvidn requested a review from mariano54 April 1, 2021 11:08
@arvidn
Use span<> and Bytes pervasively in the schemes APIs
0fd617b 
This results in a smaller interface, since the conversion vector -> Bytes is
done in the span constructor. Additionally it make the interface more flexible
in that it no longer requires the caller to allocate the array in a std::vector
@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

@mariano54
Copy link
Contributor

Is this something that we still want to do? or should we close it? @xdustinface

@arvidn
Copy link
Contributor Author

arvidn commented Nov 10, 2021

I think there are a lot of opportunities to simplify the code by making changes like this, and embracing a span type. Championing these changes are not very high on my priority list right now though.

@mariano54
Copy link
Contributor

OK, we can leave this open until someone decides to take this PR further, any help is welcome

@github-actions github-actions bot removed the stale-pr label Nov 11, 2021
@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

@justinengland
Copy link
Member

@arvidn can you help me out with resolving these merge conflicts?

@github-actions github-actions bot removed the stale-pr label Apr 26, 2023
@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

@arvidn
Copy link
Contributor Author

arvidn commented Jun 26, 2023

@justinengland sorry for the delay. this message got lost in my inbox.
I could fix this patch up, but I'm not sure it's worth the effort. I'm hoping we can move away from blspy to python bindings on top of chia-bls (rust library) once we have transitioned it to use BLST. That's some time out, but this PR is also really just a cleanup and minor performance fix.

@github-actions github-actions bot removed the stale-pr label Jun 27, 2023
@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xdustinface xdustinface xdustinface left review comments

@mariano54 mariano54 Awaiting requested review from mariano54

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
stale-pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@arvidn @mariano54 @justinengland @xdustinface