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

Add batch flag to remote-test-server #105145

Merged
merged 1 commit into from
Dec 17, 2022
Merged

Add batch flag to remote-test-server #105145

merged 1 commit into from
Dec 17, 2022

Conversation

Ayush1325
Copy link
Contributor

@Ayush1325 Ayush1325 commented Dec 1, 2022 •
edited
Loading

When using this flag, the stdout and stderr are sent in a single batch instead of being streamed. It also used Command::output instead of Command::spawn. This is useful for targets that might support std but not threading (Eg: UEFI).

Signed-off-by: Ayush Singh [email protected]

@rustbot
Copy link
Collaborator

rustbot commented Dec 1, 2022

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2022
@Ayush1325 Ayush1325 changed the title Don't use threads sequential remote-test-server Don't use threads in sequential remote-test-server Dec 2, 2022
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2022
@Ayush1325
Copy link
Contributor Author

@Mark-Simulacrum ping

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2022
@Mark-Simulacrum
Copy link
Member

r? @pietroalbini for thoughts on the best way to do this for sequential execution, since you added sequential in #102193

@pietroalbini
Copy link
Member

My need for sequential execution is, the emulator I'm working with sometimes deadlocks when multiple tests are executed at the same time, but it supports threads just fine (just can't run multiple commands in parallel).

I don't have good ideas on how to implement this without threads, other than maybe adding a --batch flag that doesn't stream the output and instead sends stdout and sterr as a whole when the command ends.

r? @Mark-Simulacrum back

@Ayush1325
Copy link
Contributor Author

@Mark-Simulacrum How about adding the --batch flag like @pietroalbini suggested and using Command::output instead of Command::spawn in it? I think that could be useful after #105458.

@Mark-Simulacrum
Copy link
Member

That sounds good to me.

@Ayush1325 Ayush1325 changed the title Don't use threads in sequential remote-test-server Add batch flag to remote-test-server Dec 11, 2022
@Ayush1325
Add batch flag to remote-test-server
2bb6bd6 
When using this flag, the stdout and stderr is sent in a single batch
instead of being streamed. It also used `Command::output` instead of
`Command::spawn`. This is useful for targets that might support std
but not threading (Eg: UEFI).

Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2022
@Ayush1325 Ayush1325 mentioned this pull request Dec 14, 2022
Closed
Mark-Simulacrum
Mark-Simulacrum reviewed Dec 17, 2022
View reviewed changes
src/tools/remote-test-server/src/main.rs
fn batch_copy(buf: &[u8], which: u8, dst: &Mutex<dyn Write>) {
let n = buf.len();
let mut dst = dst.lock().unwrap();
t!(dst.write_all(&[which, (n >> 24) as u8, (n >> 16) as u8, (n >> 8) as u8, (n >> 0) as u8,]));
Copy link
Member

Choose a reason for hiding this comment

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

As a drive-by improvement since we're changing this code, can we replace the manual shifting with writing (n as u32).encode_le_bytes() and read_u32 to use u32::from_le_bytes?

Copy link
Member

Choose a reason for hiding this comment

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

(But let's do that in a separate PR if you're up to it).

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Dec 17, 2022

📌 Commit 2bb6bd6 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2022
@bors
Copy link
Contributor

bors commented Dec 17, 2022

⌛ Testing commit 2bb6bd6 with merge 0468a00...

@bors
Copy link
Contributor

bors commented Dec 17, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 0468a00 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2022
@bors bors merged commit 0468a00 into rust-lang:master Dec 17, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 17, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0468a00): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 3.3% [1.9%, 3.8%] 7
Regressions ❌
(secondary)		 2.4% [0.7%, 4.1%] 75
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.3% [1.9%, 3.8%] 7

Cycles

This benchmark run did not return any relevant results for this metric.

@Ayush1325 Ayush1325 deleted the sequential-remote-server branch December 18, 2022 04:11
@Ayush1325 Ayush1325 mentioned this pull request Dec 19, 2022
Merged
Ayush1325 added a commit to Ayush1325/rust that referenced this pull request Dec 23, 2022
@Ayush1325
Use u32 methods instead of manual shifting
51fe248 
Switch to `to_be_bytes()` and `from_be_bytes()` instead of manual
shifting

This was suggested [`here`](rust-lang#105145 (comment))

Signed-off-by: Ayush Singh <[email protected]>
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 24, 2022
@bors
Auto merge of rust-lang#105893 - Ayush1325:remote-test-server-improve…
4f4d058 
…, r=Mark-Simulacrum

Use u32 methods instead of manual shifting

Switch to `to_le_bytes()` and `from_le_bytes()` instead of manual shifting

This was suggested [`here`](rust-lang#105145 (comment))

Signed-off-by: Ayush Singh <[email protected]>
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 24, 2022
@bors
Auto merge of #105893 - Ayush1325:remote-test-server-improve, r=Mark-…
5696986 
…Simulacrum

Use u32 methods instead of manual shifting

Switch to `to_le_bytes()` and `from_le_bytes()` instead of manual shifting

This was suggested [`here`](rust-lang/rust#105145 (comment))

Signed-off-by: Ayush Singh <[email protected]>
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#105145 - Ayush1325:sequential-remote-server, …
d8a4e2c 
…r=Mark-Simulacrum

Add batch flag to remote-test-server

When using this flag, the stdout and stderr are sent in a single batch instead of being streamed. It also used `Command::output` instead of `Command::spawn`. This is useful for targets that might support std but not threading (Eg: UEFI).

Signed-off-by: Ayush Singh <[email protected]>
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Ayush1325 @rustbot @Mark-Simulacrum @pietroalbini @bors @rust-timer