Add first nodex policy #1055
Add first nodex policy #1055
Conversation
|
Looking at the code for this, I'm thinking it would be really useful to add a new "policy" that lets us build one of these up more dynamically. It would let us factor out duplication here and let us play with the number of matches to look for and things like that. I kinda want to get this in so we have it, then build that up. |
|
Rebased. |
|
Quick resource-bench.sh numbers: So, firstnodex is about 45 times faster than hinodex, but still about 10x slower than first, at least for this little microbenchmark. |
There was a problem hiding this comment.
This looks good overall and is an elegant implementation. I did find a few things that need to be changed, which I indicated in the line-specific comments.
Two other higher-level changes:
- Please add
firstnodexand description here: - Please break up the commit such that there is one for "policies" (changes to
dfu_match_policy_factory.*pp, "testsuite" (changes tot3033-resoure-nodex.t), and "resource-query" (changes toresource-query.cpp).
| @@ -38,11 +38,13 @@ std::shared_ptr<dfu_match_cb_t> create_match_cb (const std::string &policy) | |||
| std::shared_ptr<dfu_match_cb_t> matcher = nullptr; | |||
There was a problem hiding this comment.
Please add a description to the body of the commit following the Flux "Problem:, Add:" convention.
| if (policy == FIRST_MATCH || policy == FIRST_NODEX_MATCH) { | ||
| std::shared_ptr<high_first_t> ptr | ||
| = std::make_shared<high_first_t> (); | ||
| ptr->add_score_factor (std::string ("node"), 1, 10000); | ||
| ptr->set_stop_on_k_matches (1); | ||
| if (policy == FIRST_NODEX_MATCH) | ||
| ptr->add_exclusive_resource_type ("node"); |
There was a problem hiding this comment.
I'm really impressed that this is all it takes to generate the correct behavior!
There was a problem hiding this comment.
Much as the current traverser is really complex, this is why. Dong designed it to be amazingly flexible and generic. Hopefully we can take a bit better advantage of that if we expose it a bit more.
Problem: node exclusive policies are currently slow Solution: Add a policy that accepts the first match but allocates nodes exclusively. Thanks to the flexible design of the matcher, this is entirely implemented by setting parameters on the existing implementation.
In addition to testing firstnodex, this refactors the test for hinodex and lonodex to use a common function that runs several tests parametrized on their expected output files.
Since I have to touch the string to add first-nodex, it's time to get rid of all the extra quotes and escaped newlines.
What it says on the tin, adds the new policy to the usage message.
|
It ended up being four commits, because I decided to convert the usage message string into a C++11 raw string and made that a separate commit. This should be good to go unless I botched something in rebasing it. |
Repair some odd spacing. Co-authored-by: Daniel Milroy <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1055 +/- ##
======================================
Coverage 73.4% 73.4%
======================================
Files 98 98
Lines 11888 11889 +1
======================================
+ Hits 8735 8737 +2
+ Misses 3153 3152 -1
|
trws
added
the
merge-when-passing
First cut to add a node exclusive first policy. This seems to work, refactored one of the nodex tests to not be quite so duplicative and test this too. In cases with no overlapping jobs, firstnodex is the same as hinodex, but it will only look for one match rather than scoring matches.
Need to test the performance, but this should be significantly faster than the other nodex policies.