Do not halve the number of used cores in testing #25201
Conversation
…ified the number of cores to use in testing.
test/parallel_testsuite.py
Outdated
|
|
||
| def cap_max_workers_in_pool(max_workers, is_browser): | ||
| if is_browser: | ||
| if is_browser and os.getenv('EMTEST_CORES') is None: |
There was a problem hiding this comment.
I wonder if we should maybe just no call cap_max_workers_in_pool in the case of EMTEST_CORES being set.
Also IIUC EMCC_CORES will also be used if EMTEST_CORES is not specified, so I think we would need to test both.
How about
# If a user explicitly asks for number of cores, don't artificially cap it.
if 'EMTEST_CORES' in os.environ or `EMCC_CORES` in os.environ:
return max_workers
There was a problem hiding this comment.
I wonder if we should maybe just no call
cap_max_workers_in_poolin the case ofEMTEST_CORESbeing set.
In case of the capping to max 61 cores, I think that is a good idea, because it shields the user from Python crap, which a developer might not be aware of.
I.e. say a user is using EMCC_CORES=64 and it is hanging on Windows due to the Python bug, then they have no real way of discovering that having specified > 61 cores is the cause, and our workaround will go lost on them. So it is better to force the 61 cores here, even if user sets a higher number.
There was a problem hiding this comment.
Testing both EMCC_CORES and EMTEST_CORES sounds fine, updated.
|
I think we need to adjust circleci now. We specify EMCC_CORES=4 (though there are only 2 cores on a medium instance). Before this change we had two runners going, but now there are 4. |
Sounds like the right fix yes. |
|
Oh I see, the CircleCI runners explicitly used EMCC_CORES to oversubscribe over the hardware threads? Let me post a PR. |
Do not halve the number of used cores if the user has explicitly specified the number of cores to use in testing.
It is a bit silly to have to say
export EMTEST_CORES=128to get 64 cores in actuality.