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

rts: various speedups (001) #59

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

snakpak
Copy link
Contributor

@snakpak snakpak commented Oct 10, 2018

Continued from PR #58, rebased on recent commits and fixed the sleeper -d switch.

====
This pull is a collection of patches. I thought I'd throw it out there to show where I'm at now.

It includes:

The pending whitespace tidy patch.
The sleeper patch for out of order sigcont/sigterm, rebased to current master.
Some svscan tests for the recent sigterm handler addition.
A Solaris portability fix for the new supervise-start test.
The addition of a makefifo program, strictly for use in rts. (Is mkfifo portable?)
Addition of sleeper switches to facilitate better test integration.
A proposed patch for Issue #20. Partially fixed in 7569393, but still failed to restart if the service exited 100. This patch fixes that and adds tests for exit codes 0, 1, and 100.
A change in the way supervise handles throttling, switching to iopause instead of deepsleep, eliminating the one second of unresponsiveness each time a child starts or stops.
Various rts tweaks, reducing run time to around 10 sec.
And that would bring me to the end of meeting my goals on this -- the "missing sigcont" issue and speeding up the tests -- with a couple extras thrown in just because I already had my head wrapped around it. =)

@snakpak snakpak mentioned this pull request Oct 10, 2018
@bruceg
Copy link
Owner

bruceg commented Oct 11, 2018

Note that you can redo or rebase your branch, and force push it back to the same branch on github, and github will update the existing pull request (unless it has already been pulled). No need to create new ones.

Copy link
Owner

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

I tried this patch locally (with a simpler test case) and it breaks the restart logic for services that exit 100. See my comments on bug #20. I don't think this is fixable, so I would prefer to just drop this as all the other cases are already handled.

Copy link
Owner

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

I will likely not be taking any of these before I do a release (soon).

supervise test.sv &
until svok test.sv
do
sleep 1
sleep 0
Copy link
Owner

Choose a reason for hiding this comment

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

I'm puzzled. What's the point of sleep 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's faster than sleep 1. =) Seriously, I didn't think fractional sleep times, or usleep/nanosleep, etc would be portable. So, short of adding a proprietary sleep in the distro, the only other idea I had to waste a bit of time was something like "echo foo >/dev/null".

Copy link
Contributor Author

@snakpak snakpak Oct 11, 2018

Choose a reason for hiding this comment

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

On second thought, what about something like sleeper -n -d 100000000 for a 100 ms sleep instead? At this point, adding another switch would be trivial.

@snakpak
Copy link
Contributor Author

snakpak commented Oct 11, 2018

On the race in supervise when dealing with svc -du, here's how I initially tested.

mkdir tt
cat > tt/run <<'EOF'
#!/usr/bin/perl
$SIG{TERM} = sub {};
sleep 999;
sleep 1;
exit 100;
EOF
chmod 755 tt/run

./supervise tt &
./svstat tt; ./svc -du tt; sleep 2; ./svstat tt
tt: up (pid 76231) 8 seconds, running
tt: up (pid 76268) 1 seconds, running

Isn't that what #20 was asking for?

Late Edit: I see the problem now. The -du patch breaks the non-du use of exit 100. In other words, the service restarts where it's already documented not to. Skip it for now. Piling even more ugly on that section of code probably isn't a good idea.

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.

2 participants