Closed trws closed 2 months ago
It's subtle, but the change for flux ion-R
is handled by these two lines: https://github.com/flux-framework/flux-sched/pull/1286/files#diff-5f60472bc026d952db8a60e3a2ccbc3f838188190d39ca9cd1f9ea05170ee4cdL94-R94
All the JGF handling in there is done by that other file, come to find out. It actually took me a shockingly long time to find that.
As to the breaking existing JGF, it's possible if we're using JGF in something on those systems, which we might be in some cases. I don't see where though, it looks like we're just storing R for the R key on tioga for example, so I don't think there's anything to break there. This is part of why I asked about evolving JGF at the last meeting. If we need I can make it so the reader can ingest either the new or old format and just always turn it into this internal representation, but this will not be able to emit the old format without some loss.
Ok, sounds good. Perhaps @jameshcorbett could comment if there are any systems actively using JGF (which I think he mentioned it shouldn't be an issue to upgrade)
Attention: Patch coverage is 70.83333%
with 35 lines
in your changes missing coverage. Please review.
Project coverage is 75.3%. Comparing base (
4d9fc6f
) to head (8b63d37
). Report is 9 commits behind head on master.
This includes fixes for various performance issues in the scoring api type, and resource data types (it should even speed up graph creation by making the resource types movable). The main item though is below. It takes the performance of the benchmark from #1171 from 10 jobs/s for constrained jobs and 15 jobs/s for unconstrained to 100-150 jobs/s for constrained and 150-200 jobs/s for unconstrained. For some reason this patch also consistently hits the issue in the progress bar where it will reach past the end of style, so if that comes up make sure to be testing with flux-core master.
problem: In commit https://github.com/flux-framework/flux-sched/commit/b96899b80cd8221ea0d0e08fb56fedd53d97ebec, as part of adding node exclusive, enforce_dfv was added to precisely mark unconstrained resources between the root of the containment tree and the constrained resources. This works fine, but it's a full traversal of the graph that gets run even when selecting a single core. Usually this isn't terrible performance because if the Node type is constrained then all the node vertices get skipped after the constrained one is found. When the job is only constrained on a core or socket however, or if the node isn't found quickly, it can be as bad as a full traversal. Literally 90% of the time spent matching a small single core job in the perf test case in issue https://github.com/flux-framework/flux-sched/issues/1171.
solution: The short version is: Remove enforce_dfv completely. The longer version is that instead of traversing the entire graph, we now re-use the iteration over constrained resources in dfu_impl_t::enforce and walk upward from each constrained vertex in the dominant subsystem until we find a vertex that we've already set best_k (since any above that must already have been set). That way we walk the minimum amount of the tree necessary. Honestly it's still doing more work than it should, because we're still iterating over resources that are children of exclusive resources unnecessarily, and we're having to iterate over
in_edges
rather than directly knowing the parent or subsetting on subsystem, but fixing either of those would be a meaningful refactor so I'm stopping here for now with the 15-20x speedup.fixes https://github.com/flux-framework/flux-sched/issues/1171