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

libidset: add idset_equal(3), improve efficiency of idset_count(3) #2060

Merged
merged 6 commits into from Mar 2, 2019

Conversation

@grondo
Copy link
Contributor

commented Mar 2, 2019

As a sanity check I was running a test workload to exercise the ingest->job-manager<->scheduler logic, and while doing some profiling I noticed the workload was spending 10% of the total CPU cycles in vebsucc(). As it turns out sched-simple is a heavy user of idset_count(3) (to sort nodes by least or most available), and currently that function needs to traverse the entire vEB tree for every call.

This PR adds a count member to struct idset and keeps the counted updated with idset_set and idset_clear calls.

This small change had a large improvement, so at the risk of premature optimization, I went ahead and proposed it here.

On current master, a set of 4K 1 core jobs submitted to a instance with 4K cores across 128 nodes shows

sched-bench: starting test with 4096 jobs. broker.pid=14085
sched-bench: ingested 4096 jobs in 16.13s (253.94 job/s)
sched-bench: allocated 4096 jobs in 33.526s (122.17 job/s)
sched-bench: elapsed time: 34.024s (120.38 job/s)

With the change to idset_count(3):

sched-bench: starting test with 4096 jobs. broker.pid=23102
sched-bench: ingested 4096 jobs in 16.34s (250.67 job/s)
sched-bench: allocated 4096 jobs in 20.115s (203.63 job/s)
sched-bench: elapsed time: 20.545s (199.37 job/s)

Where the ingested time is the time for t/ingest/submitbench -r 4096 -f 16 to complete, the allocated time is the difference in time between the submit event for the first job and the alloc event for the last job, and the elapsed time is the total time from the start of submitbench and all jobs getting allocated resources. (ingest and "scheduling" are happening together here)

While adding new tests, I noticed that there was already an idset comparator function within the unit test, and since this was needed for sched-simple, I moved that code into idset_equal(3) now exported by libidset.

Man page entry for idset_equal is also added, but I can't add it to the manpage stubs for idset_create.adoc because of the old problem where a2x won't generate more than 9 stubs for a manpage (any workaround for that?)

@grondo grondo changed the title libidset: add idset_equal(3), improve efficiency of idset_count(3) wip: libidset: add idset_equal(3), improve efficiency of idset_count(3) Mar 2, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Added wip: prefix to this PR until I figure out manpage generation.

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

This looks great!

There are some other man pages that have missing stubs as well, so fixing/working around that would be nice. I don't really have any ideas on how to address it, though maybe encode/decode could be split out to their own adoc?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Hm: Looks like asciidoc is EOL in 2020, with asciidoctor or asciidoc-py3 as suggested replacement:

https://github.com/asciidoc/asciidoc

Until then (or until wider availability of asciidoctor >= 1.5.7), though, splitting .adoc files is the only thing I can think of as well.

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

IMHO I think this can go in with missing manpage stubs. There are a few others that are missing stubs I believe, so when we rid ourselves of asciidoc/a2x we can do an audit and fill them all in.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Ok (I do need to reword or update spelling dictionary).

I was going down the route of trying to make a libidset.adoc.in that generated identical idset_create.adoc and idset_encode.adoc each with different stubs listed (but same content). Would be nice not to have to do that..

grondo added 6 commits Mar 1, 2019
Problem: idset_count(3) needs to iterate the entire vEB tree to
get a count of entries in the set on each call, and this is
(relatively) slow. Considering use cases of idset_count(3) such
as in a sort algorithm, an improvement here may be warranted.

Add a count member to the idset structure and keep the embeeded
count up to date within the idset_set/clear set of calls.

This unfortunately adds some slight overhead to the set and clear
operations, since each integer argument to set/clear has to
first be checked for membership (or lack thereof) in the idset
so that the count can be appropriately updated.
Now that there is a newer implementation of idset_count(3), add
some extra checks to handle possible corner cases where the internal
idset count accounting could go wrong.
Add idset_equal(3) to libidset to compare two idsets for equality.
Add a set of specific tests in the libidset unit tests for
idset_equal(3) functionality.
Now that there exists an `idset_equal()` implementation in libidset,
use this function instead of a custom comparator in the shed-simple
rlist.c code.
Document the idset_equal(3) function call.

Add a note to Makefile.am that the manpage stub idset_equal.3
can not be generated because it exceeds the max 9 stubs for
asciidoc.
@grondo grondo force-pushed the grondo:libidset-updates branch from d3fe1d0 to 880e8eb Mar 2, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Ok, just forced an update that adds a note to doc/man3/Makefile.am to note why idset_equal.3 is not generated, and slightly rewords documentation so that a spelling dict update isn't needed.

@grondo grondo changed the title wip: libidset: add idset_equal(3), improve efficiency of idset_count(3) libidset: add idset_equal(3), improve efficiency of idset_count(3) Mar 2, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Mar 2, 2019

Codecov Report

Merging #2060 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2060      +/-   ##
==========================================
+ Coverage   80.42%   80.44%   +0.02%     
==========================================
  Files         191      191              
  Lines       30273    30286      +13     
==========================================
+ Hits        24346    24363      +17     
+ Misses       5927     5923       -4
Impacted Files Coverage Δ
src/modules/sched-simple/rlist.c 81.49% <100%> (-0.22%) ⬇️
src/common/libidset/idset.c 96.35% <100%> (+0.55%) ⬆️
src/broker/modservice.c 78.84% <0%> (-0.97%) ⬇️
src/common/libflux/message.c 81.51% <0%> (ø) ⬆️
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️
src/common/libutil/veb.c 98.85% <0%> (+0.57%) ⬆️
src/modules/barrier/barrier.c 78.62% <0%> (+2.06%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

looks good, thanks!

@garlick garlick merged commit bc15084 into flux-framework:master Mar 2, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.42%)
Details
codecov/project 80.44% (+0.02%) compared to 970c98e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo grondo deleted the grondo:libidset-updates branch Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.