[bazel] allocate additional core per verilator test to allow for harness and overhead #16915
Conversation
|
I think a 20% performance hit might be worth the simplicity this offers relative to #16913 |
|
I'm kind of surprised that you can allocate a fractional number of CPUs. What does that mean? Is this the best reference for the |
|
I was confused, the resources folks at bazel want the extra resources to be a float, but the cpus appears to be parsed as an int, now I need to figure out why this appeared to work. :/ |
|
Looks like it runs 18 simultaneously, so it's parsing as 4, not 4.5. There's a bit of validation code that appears to be having some trouble with this. |
drewmacrae
changed the title
drewmacrae
changed the title
drewmacrae
force-pushed
the
conservative_resource_allocation
branch
from
5fa02a0 to
e99a6f2
Compare
|
ci/scripts/run-verilator-tests.sh overrides the effect of this commit so CI verilator jobs wont be protected by it. They would be safer if they limited --local_test_jobs to 4 or allocated 5 CPUs per test runner instead of 4. |
drewmacrae
force-pushed
the
conservative_resource_allocation
branch
from
e99a6f2 to
e6bd406
Compare
| @@ -152,7 +152,7 @@ def verilator_params( | |||
| "@//hw:verilator", | |||
| "@//hw:fusesoc_ignore", | |||
| ] | |||
| required_tags = ["verilator", "cpu:4"] | |||
| required_tags = ["verilator", "cpu:5"] | |||
There was a problem hiding this comment.
This probably ought to depend on the number of threads for which the verilated model is compiled, but it's fine for now.
There was a problem hiding this comment.
It definitely ought to, but the tags don't like being updated when that information is known to Bazel as far as I can tell. So I got stuck trying to implement that.
…overhead An alternative to lowRISC#16913 to prevent harnesses from getting pre-empted by a simulation load. We also stop overriding resource based allocation by removing --local_test_jobs arg and update documentation Signed-off-by: Drew Macrae <[email protected]>
drewmacrae
force-pushed
the
conservative_resource_allocation
branch
from
e6bd406 to
441b09c
Compare
drewmacrae
added
the
Status:Ready to merge
An alternative to #16913 to prevent harnesses from getting pre-empted by a simulation load.
Running 17 verilator tests and 1 fpga test on 72 cores will cause problems unless the test harnesses are prioritized above the verilated model. An alternative to this prioritization is to recognize that verilated tests require 4 cpus for the simulation alone and further resources for appropriate overhead. Empirically 17/72ths of a core is too little to prevent issues but 16/72nds is safe, but bazel requires cpu: (it parses up to the decimal and stops without throwing an error).
Won't fix: #16831 because it uses local_test_job based scheduling that overrides resource constrained scheduling. For #16831, similar methods would cause a 30% performance hit.
Signed-off-by: Drew Macrae [email protected]