Draft: Update core assignment algorithm in benchexec/resources.py #892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CGall42
added
the
resource allocation
PhilippWendler
requested changes
Jan 26, 2023
There was a problem hiding this comment.
This is a preliminary review with some hints, mostly on code style and documentation. Please fix these issues and provide documentation, such that a full review becomes possible and makes sense.
As part of this (as first step actually), please format the source code with the formatter black and make sure to use it in the future for each commit. Consistent code formatting is a big help for readability.
And please also have a look at all the other CI failures. After each commit these checks will run automatically, so always check whether CI is green for your most recent commit. The check check-format is fixed automatically if you use the code formatter, and reuse only complains about the missing copyright header. But flake8 and pytype provide hints about potential errors in your code. And of course the unit-tests checks just execute our tests.
This method adds a "root" hierarchy level, if the system topology doesnt have one. Necessary for iterating through the whole topology.
now chooses the right starting core for the next thread
To be able to remove the fixed functions (and replace them with one which takes arbitrary numbers of cores), all calls to the existing methods are redirected to a new method. This removes all direct references to mainAssertValid, allowing us to refactor the code without having to modify each test at the same time.
Currently, there are two different kinds of assert statements in the code - assertValid, which is quite straightforward, and mainAssertValid, which performs additional checks. As the additional checks in mainAssertValid only further constrain when a test passes the assertion, we safely can change the existing calls to assertValid.
This reverts commit dd06fcb.
…rRun This also adds the required values which were inherited from TestCpuCoresPerRun_singleCPU With this, all classes are derivated from TestCpuCoresPerRun, allowing us to use decorators more easily later
Currently, all tests are limited to 1-4, 8 CPUs used with adding test cases taking a bit of work. This decorator dynamically adds a test case to test if the actual assignment matches the expected assignment to a test class. It doesn't use the fixed versions of xxxCoresPerRun, which allows us to remove them later entirely.
Note that one test is commented out, as the decorator unexpectedly fails where the actual test succeeds for at this time unknown reasons. This commit adds redundant tests - this is temporary, as commits in the near future commits will remove the old testing logic, which the decorator replaces.
In the existing testsuite, an overwritten test_fourCoresPerRun method actually used the values of the 3 cores version.
In this case, the preexisting testsuite had a bug, which explains why the results differed from the new decorator based one
As the decorator based tests are identical with the existing code and also all pass, we can remove the old testing logic
This commit again temporarily increases the raw amount of tests significantly, as both the old and new versions still exist
As the expect_invalid decorator performs equal assertions, we can remove the individual statements from the test classes
The added assertion is required to ensure the new definitions (which allow arbitrary CPU layouts¹) are actually identical to the existing ones. It will be removed together with the old definitions after the refactoring Note that the definition is based on the physical CPU layout, with the number of virtual cores per core being the second value of the (<cpu_layout>, <virtual_cores_per_core>) tupel. ¹) with the only restriction that we require CPUs in multi CPU systems to be identical, i.e. the layout needs to be symmetric
As noted in the previous commit, those definitions are based on the physical layout of the CPUs, with the number of hyperthreading / virtual cores per core being a seperate argument. This has the upside that the machine definitions do not need to be internally consistent, i.e. we do not need to take care if the total number of cores is actually possible with the number of virtual cores.
As the assertions show that they are equal for the existing tests, we can replace the existing logic to translate the old definitions and directly use the new ones. Also removes debug print statements and replaces the use of num_of_hyperthreading_siblings, so we can use the one in the machine definition (which we later can easily use with pytest parameterization).
Additionally, we temporarily as a shim set self.num_of_cores based on the physical cores times the number of virtual cores per core
As we now only use the single list-based machine definition, the specific layers are redundant and unused
The pytest-based assertion will replace the use of the assertInvalid method
As we use the equivalent pytest-based assertion, the old assertion is redundant
The _test_nCoresPerRun method is a remainder of prior refactoring - as it only contains a single call to another function, we can remove and inline its content safely
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Referring to issue #748, the core assignment can now handle additional hierarchy layers (such as a shared L3 cache).
The addition of further layers can be implemented without knowing the exact topology of a machine - the hierarchy of the layers (CPUs, NUMA nodes, L3 caches, hyperthreading, etc) is determined by the algorithm.
Fixes #748
Fixes #850
Kernel documentation: