-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update nvm.sh to detect number of cores on aix #1319
Conversation
…t just number of processors
@@ -1789,7 +1789,7 @@ nvm_get_make_jobs() { | |||
elif [ "_$NVM_OS" = "_sunos" ]; then | |||
NVM_CPU_THREADS="$(psrinfo | wc -l)" | |||
elif [ "_$NVM_OS" = "_aix" ]; then | |||
NVM_CPU_THREADS="$(lsconf | command grep 'Number Of Processors:'| command awk '{print $4}')" | |||
NVM_CPU_THREADS="$(pmcycles -m | wc -l)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand - does AIX abstract over "processors" such that programs should be using N - 1 threads in order to avoid locking up the machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason that it should be any different to other platforms. When I added AIX I couldn't get a command that counted threads easily but have now found that this command does exactly that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but the purpose of this check is to ensure that there is always at least one CPU available to process commands besides nvm install -s
, to avoid locking up the machine. Using the thread count will not ensure there is always at least one processor available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that not why you subtract 1 at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But by starting with the number of threads, we might have 4 CPUs and 20 threads, and we need to only ever run 3 processes in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibfahn that is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
essentially, before I could only get the number of physical cpu's... now I can get the number of CPU cores
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point; perhaps the variable name needs renaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should probably be NVM_CPU_CORES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
@ljharb Is this change ready to approve? |
@GeorgeAdams95 after a rebase due to #1326, the code blockers would be removed, but conceptually we'd been discussing whether this change actually makes sense. The intention here is to count processor cores, not threads. |
@ljharb This PR does exactly that! before it was counting physical cpu's now it counts cores. Before the output was 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! LGTM, pending a rebase.
@ljharb updated PTAL |
Currently nvm is detecting the number of processors available for building on aix. I have managed to improve this to get the number of threads