Page MenuHomePhabricator

[lit] Use os.cpu_count() to cleanup TODO
ClosedPublic

Authored by yln on Jan 14 2021, 5:17 PM.

Details

Diff Detail

Event Timeline

yln created this revision.Jan 14 2021, 5:17 PM
yln requested review of this revision.Jan 14 2021, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 5:17 PM

Didn't look at the diff, but there's already code in the tree that requires python3 (eg subprocess.DEVNULL in compiler-rt/test/lit.common.cfg.py). So that part is fine.

jdenny added inline comments.Jan 21 2021, 2:05 PM
llvm/utils/lit/lit/util.py
118–128

I'm comparing the following documentation:

It seems like SC_NPROCESSORS_ONLN is closer to len(os.sched_getaffinity(0)) than os.cpu_count(). I'm not familiar with these routines, so I might be misunderstanding.

yln updated this revision to Diff 318596.Jan 22 2021, 11:46 AM

Address Joel's comments.

yln marked an inline comment as done.Jan 22 2021, 11:50 AM
yln added inline comments.
llvm/utils/lit/lit/util.py
118–128

That's a good point. What we are really interested in is the number of usable cores. Changed function to the following:
len(os.sched_getaffinity(0)) get number of usable cores on platforms that support this query
os.cpu_count() get total number of cores (with a fallback to 1)

jdenny accepted this revision.Jan 22 2021, 12:23 PM

LGTM

This revision is now accepted and ready to land.Jan 22 2021, 12:23 PM
This revision was automatically updated to reflect the committed changes.
yln marked an inline comment as done.
keith added a subscriber: keith.Jan 25 2021, 12:27 PM

Looks like this broken a build http://green.lab.llvm.org/green/job/ZorgTests/49794/console

Not sure if that build should be updated to use python3, or this should be reverted

yln added a comment.Jan 25 2021, 1:35 PM

Reverting until we can fix the bot (since it's low priority).