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

Locked stdin and out for rust sbp2json #1161

Merged
merged 24 commits into from
Jun 8, 2022
Merged

Locked stdin and out for rust sbp2json #1161

merged 24 commits into from
Jun 8, 2022

Conversation

adrian-kong
Copy link
Contributor

Description

Locks the stdin and stdout for sbp2json

JIRA Reference

https://swift-nav.atlassian.net/browse/DEVINFRA-790

Copy link
Contributor

@notoriaga notoriaga left a comment

Choose a reason for hiding this comment

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

for edification this avoids acquiring a lock on a mutex for each read/write. the lock is always uncontested so in reality it's not a big deal (especially after the std mutex improvements). you still see a small perf boost though

hyperfine 'sbp2json <./tests/data/regression/2020-06-25-BayLoop/STEP_v2_SR_L2_OSR_staging_10Hz_GNSSOnly_AmotechL2/25-190704.sbp >/dev/null'
Benchmark 1: sbp2json <./tests/data/regression/2020-06-25-BayLoop/STEP_v2_SR_L2_OSR_staging_10Hz_GNSSOnly_AmotechL2/25-190704.sbp >/dev/null
  Time (mean ± σ):      5.132 s ±  0.027 s    [User: 4.896 s, System: 0.236 s]
  Range (min … max):    5.105 s …  5.194 s    10 runs

hyperfine 'sbp2json <./tests/data/regression/2020-06-25-BayLoop/STEP_v2_SR_L2_OSR_staging_10Hz_GNSSOnly_AmotechL2/25-190704.sbp >/dev/null'
Benchmark 1: sbp2json <./tests/data/regression/2020-06-25-BayLoop/STEP_v2_SR_L2_OSR_staging_10Hz_GNSSOnly_AmotechL2/25-190704.sbp >/dev/null
  Time (mean ± σ):      4.962 s ±  0.044 s    [User: 4.712 s, System: 0.249 s]
  Range (min … max):    4.900 s …  5.037 s    10 runs

@notoriaga
Copy link
Contributor

notoriaga commented Jun 7, 2022

I think the dockerfile in the rust folder needs to be updated to a newer version of rust. I'm guessing since 1.55 some borrowing rule was relaxed because the code compiles in the other workflows which use the latest version. can't find what actually changed though 🤔

@adrian-kong adrian-kong requested a review from a team as a code owner June 7, 2022 04:58
@silverjam
Copy link
Contributor

@adrian-kong Next thing we can try is to use MUSL to work around this issue... basically anywhere we call cargo build --release add --target=x86_64-unknown-linux-musl you'll probably need to reintroduce the installation of musl-tools to the Dockerfiles

@adrian-kong
Copy link
Contributor Author

hmmmm if we change the target, it changes the target path from target/release -> target/x86_64-unknown-linux-musl/release, should we just change all the OS to use their specific binary targets so it wont just be target/release

@silverjam
Copy link
Contributor

hmmmm if we change the target, it changes the target path from target/release -> target/x86_64-unknown-linux-musl/release, should we just change all the OS to use their specific binary targets so it wont just be target/release

I think it only does this if you're cross compiling, or manually specifying the build target.

@silverjam
Copy link
Contributor

I think the latest failure is from the upgrade of hyperfine, the format of the json it outputs has probably changed... you'll need to run it locally to see what the new structure looks like (or rollback the upgrade if there wasn't a reason it was needed).

Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

Some slight slowdowns due to the switch to musl but I think we can live with that to get more universally compatible binaries

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