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

shell: fix delay in completion of jobs with a single shell rank #4159

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Feb 24, 2022

Commit 1d08bf7 modified how the shell output plugin detects completion, but it introduced a performance issue for single rank jobs, which now wait at least output.batch-timeout seconds before the output plugin flushes pending events and exits the reactor.

This PR fixes that issue by having the rank 0 shell add a reference count per task - so that once all tasks are complete the output eventlogger can be flushed and exit immediately.

Without this fix single rank jobs have a minimum runtime of ~500 ms, which affects single node job throughput.

Problem: The rank 0 job shell normally waits for an "eof" message
to the shell output.write service in order to know when it can
flush pending events in eventlogger and release the shell completion
reference for the output plugin. However, this reference counting is
skipped when there is only one shell involved in the job. This causes
a delay after all tasks output is closed of up to output.batch-timeout
seconds (.5s by default).

Instead of ignoring the contribution of the rank 0 shell to the output
completion reference count, have each local task contribute to the
reference count on this shell, and decrement the reference count for
these tasks in the task.exit callback. This allows the output plugin
to flush output immediately when the last task is complete. Since
tasks are not considered complete until they have exited *and* all
streams have EOF, this should be perfectly safe.
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM! Good find!

@grondo
Copy link
Contributor Author

grondo commented Feb 28, 2022

Rebased. Thanks! I'll set MWP.

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #4159 (098d3ce) into master (bbe5d22) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 098d3ce differs from pull request most recent head a68800c. Consider uploading reports for the commit a68800c to get more accurate results

@@            Coverage Diff             @@
##           master    #4159      +/-   ##
==========================================
+ Coverage   83.43%   83.46%   +0.02%     
==========================================
  Files         379      378       -1     
  Lines       63420    63427       +7     
==========================================
+ Hits        52915    52937      +22     
+ Misses      10505    10490      -15     
Impacted Files Coverage Δ
src/shell/output.c 77.03% <100.00%> (+0.22%) ⬆️
src/common/libkvs/kvs_commit.c 76.92% <0.00%> (-1.10%) ⬇️
src/cmd/builtin/overlay.c 90.23% <0.00%> (-0.97%) ⬇️
src/modules/job-archive/job-archive.c 60.66% <0.00%> (-0.74%) ⬇️
src/modules/cron/cron.c 82.47% <0.00%> (-0.45%) ⬇️
src/common/libflux/future.c 87.60% <0.00%> (-0.28%) ⬇️
src/modules/job-info/guest_watch.c 76.75% <0.00%> (-0.28%) ⬇️
src/common/libutil/errprintf.c
src/modules/job-manager/jobtap.c 84.65% <0.00%> (+0.09%) ⬆️
src/common/libsdprocess/sdprocess.c 69.79% <0.00%> (+0.12%) ⬆️
... and 9 more

@mergify mergify bot merged commit b857803 into flux-framework:master Feb 28, 2022
@grondo grondo deleted the slowrun branch February 28, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants