Skip to content

fix: only add '-Ctarget-feature=+sse4.1,+sse4.2' flags for x86_64 #8305

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
Merged
merged 2 commits into from
Jan 9, 2023
Merged

fix: only add '-Ctarget-feature=+sse4.1,+sse4.2' flags for x86_64 #8305

merged 2 commits into from
Jan 9, 2023

Conversation

pugachAG
Copy link
Contributor

@pugachAG pugachAG commented Jan 6, 2023 •
edited
Loading

With #8284 we upgraded cargo version to 1.66, so rust-lang/cargo#11114 is available. This PR is the same as #7130, but we can safely merge it now.

See #7650 for more context.

In order to test it I've used the code from librocksdb-sys/build.rs as part of neard/build.rs:

 fn main() {
-    if let Err(err) = try_main() {
-        eprintln!("{}", err);
-        std::process::exit(1);
+    let target_feature = std::env::var("CARGO_CFG_TARGET_FEATURE").unwrap();
+    let target_features: Vec<_> = target_feature.split(',').collect();
+    if target_features.contains(&"neon") {
+        panic!("FAIL: {target_feature}");
+    } else {
+        panic!("OK: {target_feature}");
     }
 }

It was was tested on M1 macbook, so I've changed the feature to neon (which is enabled by default), my .cargo/config.toml is as follows:

-[target.'cfg(target_arch = "x86_64")']
-rustflags = ["-Ctarget-feature=+sse4.1,+sse4.2", "-Cforce-unwind-tables=y"]
+[target.'cfg(target_arch = "aarch64")']
+rustflags = ["-Ctarget-feature=-neon", "-Cforce-unwind-tables=y"]

neon was successfully removed when using 1.66 toolchain:

OK: crc,...,vh

while it was still present with 1.65:

FAIL: aes,..,neon,...,vh

This proves that rustflags are passed as env variable to build.rs in 1.66, which was not the case in 1.65

@pugachAG pugachAG linked an issue Jan 6, 2023 that may be closed by this pull request
Don't enable SSE for wasm compilation #7650
Closed
@pugachAG pugachAG requested review from akhi3030 and jakmeier January 6, 2023 23:11
@pugachAG pugachAG marked this pull request as ready for review January 6, 2023 23:11
@pugachAG pugachAG requested a review from a team as a code owner January 6, 2023 23:11
@akhi3030
Copy link
Contributor

akhi3030 commented Jan 9, 2023

Thanks for this @pugachAG! Can we somehow make sure that the appropriate flags are still being passed to rocksdb? I.e. we are not getting a repeat of #7648. I believe @marcelo-gonzalez had originally noticed a performance degradation so we could repeat his performance test as well.

@pugachAG
Copy link
Contributor Author

pugachAG commented Jan 9, 2023

@akhi3030 good point, I've aded my test plan to the PR description, please take a look.

nagisa
nagisa reviewed Jan 9, 2023
View reviewed changes
.cargo/config.toml

[target.'cfg(target_arch = "x86_64")']
rustflags = ["-Ctarget-feature=+sse4.1,+sse4.2", "-Cforce-unwind-tables=y"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the force-unwind-tables need to be duplicated?

Copy link
Contributor Author

@pugachAG pugachAG Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes, I've explicitly verified the rustc arguments emitted by cargo build -v.

also found the confirming issue: rust-lang/cargo#5376

@marcelo-gonzalez
Copy link
Contributor

Thanks for this @pugachAG! Can we somehow make sure that the appropriate flags are still being passed to rocksdb? I.e. we are not getting a repeat of #7648. I believe @marcelo-gonzalez had originally noticed a performance degradation so we could repeat his performance test as well.

I think what I saw was archival nodes being very slow, and I sshed into one and ran RUST_LOG=trace neard view-state apply --height {} for some height around the HEAD. If you did this w a binary built from right before #7130, and then with one from right after, you would see a big performance difference

@near-bulldozer near-bulldozer bot merged commit 4820b04 into near:master Jan 9, 2023
@marcelo-gonzalez
Copy link
Contributor

marcelo-gonzalez commented Jan 10, 2023 •
edited
Loading

just tried running view-state apply on a testnet archival node like I mentioned before and it looks all good. Actually is a lot faster after this commit

@pugachAG
Copy link
Contributor Author

@marcelo-gonzalez thanks for double-checking this on testnet!

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

@nagisa nagisa nagisa left review comments

@jakmeier jakmeier Awaiting requested review from jakmeier

+1 more reviewer

@akhi3030 akhi3030 akhi3030 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't enable SSE for wasm compilation
4 participants
@pugachAG @akhi3030 @marcelo-gonzalez @nagisa