Reduce sh postRun overhead by combining 3 shSync calls into 1#265
Reduce sh postRun overhead by combining 3 shSync calls into 1#265willr3 merged 1 commit intoHyperfoil:masterfrom
Conversation
willr3
left a comment
willr3
left a comment
There was a problem hiding this comment.
I think combining multiple shSync would be helpful, where possible, but this PR introduces several functional edge cases that I suspect would break current user functionality and introduce uncertainty into the execution state for user scripts.
| context.getShell().isPromptShell(getPreviousPrompt()) && | ||
| context.getShell().getHost().isShell()) | ||
| context.getShell().getHost().isShell() && | ||
| shouldCheckExit(context)) |
There was a problem hiding this comment.
pwd is needed even if exit code is not being checked.
| * With the default 100ms SuffixStream delay, this adds ~300ms+ per sh command. | ||
| * These tests quantify that overhead in realistic scenarios. | ||
| */ | ||
| public class ShCommandOverheadTest extends SshTestBase { |
There was a problem hiding this comment.
This looks like another class that was useful when considering the change but I do not understand how it would be helpful to maintain this as part of the qDup test suite.
| while (!parsed && retry < 5 && combined != null && !combined.isBlank() && context.getShell().isReady() && !context.isAborted()) { | ||
| combined = context.getShell().shSync("echo \"${__qdup_ec}:::$(pwd)\""); | ||
| if (combined != null && combined.contains(":::")) { | ||
| String[] parts = combined.trim().split(":::", 2); |
There was a problem hiding this comment.
using split makes the assumption that the deliminator is not part of the pwd output. That is likely true but using indexOf would avoid that potential issue.
|
|
||
| // Warmup: discard first measurement to avoid SSH channel setup overhead | ||
| calibrating = true; | ||
| shSync("echo qdup_ping"); |
There was a problem hiding this comment.
This assumes echo is a valid command on the remote host. That is likely valid for the majority of systems but have automation where that is not correct. We have dealt with this in qDup in two ways:
- have a configuration option on the host definition to override the default command.
- Only run this command if
host.isShell()=trueand otherwise use a default value.
- Always track pwd in postRun even when exit code checking is disabled, so queue-download relative paths work correctly in all contexts - Revert lazy pwd fetch in QueueDownload to preserve watch/observer context compatibility (shSync cannot be called from observers) - Use indexOf instead of split for parsing "exitCode:::pwd" delimiter to handle the unlikely case of ::: appearing in directory names - Handle !parsed case explicitly by logging error and aborting to avoid running in an unknown execution state - Remove benchmark test classes (ShCommandOverheadTest, ShellOverheadBenchmarkTest) as they are profiling tools not suitable for the main test suite
|
@willr3 I added another commit which addresses your comments I hope :) |
stalep
force-pushed
the
perf_improvements
branch
from
23452bd to
c496ee4
Compare
|
@willr3 what do you think about this? |
Previously, after each sh command, postRun() made 3 sequential shSync
calls: exit code capture, pwd, and exit code restore. Each shSync blocks
until prompt detection fires, so this added 3 round-trips of overhead
per command.
Combine all three into a single command:
export __qdup_ec=$?; echo "${__qdup_ec}:::$(pwd)"; (exit $__qdup_ec)
This captures exit code, pwd, and restores exit code in one round-trip.
The response is parsed using indexOf to handle the unlikely case of :::
appearing in directory names. If parsing fails (e.g. noisy output from
concurrent processes), retries re-echo the saved variable. If all
retries fail, the run is aborted with a clear error message rather than
continuing in an unknown state.
stalep
force-pushed
the
perf_improvements
branch
from
c496ee4 to
ba97ba5
Compare
2 participants
The postRun() method previously made 3 sequential shSync calls after every sh command (exit code capture, pwd, exit code restore), each blocked by the 100ms SuffixStream prompt detection delay, adding ~300ms of overhead per command.
Combine them into a single shSync call that captures exit code and pwd in one output delimited by ":::", reducing overhead to ~100ms (~3x faster). Fallback retry logic for noisy output is preserved.
Also adds benchmark tests to measure and track shell overhead.