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 features #238

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Fix features #238

merged 3 commits into from
Oct 5, 2023

Conversation

nazar-pc
Copy link
Contributor

First commit removes features of blst that shouldn't be necessary (and shouldn't exist in blst in the first place) as described in #233 (comment).

Second commit fixes regression from #232 that caused hex dependency to pull standard library in (annoyingly wasm32-unknown-unknown does have a standard library, although an incomplete one, so it didn't cause CI failure).

@belijzajac
Copy link
Collaborator

belijzajac commented Sep 26, 2023

@nazar-pc
Copy link
Contributor Author

Yeah, will fix, do you otherwise agree it is better to remove those features though or should provide more details?

@sauliusgrigaitis
Copy link
Member

AFAIK runtime detection costs a bit of performance, can't recall if is it noticeable. If that's true, then we may leave option for user to force settings instead of runtime detection.

@belijzajac
Copy link
Collaborator

belijzajac commented Sep 26, 2023

c-kzg-4844 removed features for portable and force-adx ethereum/c-kzg-4844#338, so maybe we should too?

@nazar-pc
Copy link
Contributor Author

AFAIK runtime detection costs a bit of performance, can't recall if is it noticeable. If that's true, then we may leave option for user to force settings instead of runtime detection.

There is no runtime cost, it is all decided at compile time. blst will enable support for necessary features based on the target platform, so RUSTFLAGS="-C target-cpu=x86-64-v2" will not use ADX and RUSTFLAGS="-C target-cpu=skylake" will.

blst is essentially infecting all downstream projects with its features, but at least they support target-cpu for Rust since supranational/blst@9bf52f8

@sauliusgrigaitis
Copy link
Member

Interesting, I thought they added runtime detection recently supranational/blst#10.

@nazar-pc
Copy link
Contributor Author

Looks like they are only used in portable builds, so compiling without selecting portable flag should give you the same result as before.

@nazar-pc
Copy link
Contributor Author

Is there anything else I should do here? With this we should finally be able to use upstream version of this library at Subspace Network, had to patch it all along prior to this.

@sauliusgrigaitis sauliusgrigaitis merged commit 839c76f into grandinetech:main Oct 5, 2023
20 checks passed
@sauliusgrigaitis
Copy link
Member

Seems it broke the build. I'm reverting.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Oct 5, 2023

Looks like -C target-cpu=x86-64 needs to be applied at a different position in the command.

@nazar-pc nazar-pc deleted the fix-features branch December 13, 2023 10:37
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