Rename Sys.CPU_CORES to more explict Sys.CPU_LOGICAL_CORES #27856

juliohm · 2018-06-29T03:42:56Z

This pull request renames Sys.CPU_CORES to the more explicit name Sys.CPU_LOGICAL_CORES, and the corresponding env variable JULIA_CPU_CORES to JULIA_CPU_LOGICAL_CORES.

I've experienced cases of users on Windows using CPU_CORES thinking of physical cores, and then having their simulations running 1000x slower because hyperthreading is not supported sometimes.

The changes also leave the door open for a future Sys.CPU_PHYSICAL_CORES with the actual physical cores.

Please let me know if something is missing or if the changes are not appropriate.

ararslan · 2018-06-29T03:45:56Z

Seems like we could just better document that Sys.CPU_CORES is the number of logical cores.

juliohm · 2018-06-29T03:50:26Z

Sounds like an alternative, but do you think it would solve the misinterpretation effectively? And if we need to add the actual physical cores in the future, how should it be called?

StefanKarpinski · 2018-06-29T06:23:04Z

Calling it Sys.CPU_LOGICAL_CORES seems reasonable to me. Between that and a hypothetical Sys.CPU_PHYSICAL_CORES, you have an upper and lower bound on the amount of actual concurrency you can possibly hope to get.

nalimilan · 2018-06-29T11:53:16Z

Can you look at what terminology other software use?

One advantage of keeping CPU_CORES is that it reflects the number of threads we use by default for OpenBLAS (IIRC), so it's a reasonable default for users. If it doesn't work in some cases, maybe it just needs more documentation. BTW, 1000× really sounds like a lot, do you have details on that workflow?

juliohm · 2018-06-29T16:45:42Z

Thank you all for the comments. I like the shorter name CPU_CORES as well, however different software use it with different meaning. Having seen the efforts in Julia to make names more explicit, I thought this one would also be a good candidate to fix. The current documentation already mentions logical cores, but I think not many people in the sciences are aware of this differentiation logical vs. physical cores and the impacts of hyperthreading to performance.

Regarding the slow down, I think I've exaggerated the number, sorry. I could only find an old issue with a 10x slowdown that we took a while to figure out. After back and forth with many hypothesis falsified it was just a misinterpretation of the variable name: #16570

nalimilan · 2018-06-29T16:55:13Z

Doesn't #16570 indicate that FFTW should use different defaults on Windows? Skimming that discussion, it seems the user hasn't explicitly used CPU_CORES, right?

juliohm · 2018-06-29T17:01:22Z

In that code, I was setting the number of threads to CPU_CORES by default for the user because I had misinterpreted it as physical cores. This number of threads was then fed to FFTW inside of my package.

I fixed the issue later by depending on Hwloc.jl which uses the terminology "Core" for physical cores and "PUs" for logical units:

import Hwloc
topology = Hwloc.topology_load()
counts = Hwloc.histmap(topology)
ncores = counts[:Core]
npus = counts[:PU]
println("This machine has $ncores cores and $npus PUs (processing units)")

The CpuId.jl package uses cpucores for physical cores and cpucores_total for logical cores. Just based on these two examples, we see how the terminology varies considerably.

StefanKarpinski · 2018-06-30T00:35:50Z

having their simulations running 1000x slower because hyperthreading is not supported sometimes.

This doesn't really make much sense to me:

If hyperthreading isn't supported then "physical cores" = "logical cores".
A 1000x slowdown from oversubscribing the number of physical cores seems unlikely.

juliohm · 2018-06-30T00:43:01Z

Yes, as I wrote, I definitely exaggerated there. But it is a slow down that converts 1 hour in a day or so. Significant

…

On Fri, Jun 29, 2018, 5:36 PM Stefan Karpinski ***@***.***> wrote: having their simulations running 1000x slower because hyperthreading is not supported sometimes. This doesn't really make much sense to me: - If hyperthreading isn't supported then "physical cores" = "logical cores". - A 1000x slowdown from oversubscribing the number of physical cores seems unlikely. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMLbcWjhjtu_46cjG-mWVrwxxOPK64Wks5uBsf1gaJpZM4U8ZKy> .

juliohm · 2018-07-01T17:13:10Z

Based on the positive hands in @StefanKarpinski 's comment, I think most agree the change would improve clarity.

The build is failing because I am using the macro @deprecate incorrectly. Could you please advise?

@deprecate Sys.CPU_CORES Sys.CPU_LOGICAL_CORES

fredrikekre · 2018-07-01T17:25:49Z

@eval Sys @deprecate_binding CPU_CORES CPU_LOGICAL_CORES false

juliohm · 2018-07-01T22:13:01Z

base/deprecated.jl

@@ -1732,6 +1732,9 @@ end
 @deprecate mapfoldl(f, op, v0, itr) mapfoldl(f, op, itr; init=v0)
 @deprecate mapfoldr(f, op, v0, itr) mapfoldr(f, op, itr; init=v0)

+# PR #27856
+@eval Sys @deprecate_binding CPU_CORES CPU_LOGICAL_CORES false


@fredrikekre did as you suggested but the CI build is saying that @deprecate_binding is not defined

Base.@deprecate_binding

juliohm · 2018-07-02T23:15:41Z

The error in Travis now is UndefVarError(:CPU_LOGICAL_CORES). Could you please advise on other potential issues in this PR?

KristofferC · 2018-07-03T09:30:04Z

It seems the variable CPU_CORES is not created until the __init__ function is run, which hasn't happened yet when the deprecation macro is run.

StefanKarpinski · 2018-07-03T12:56:55Z

Workflow I use:

just do the rename and push that and make sure everything passes
add the deprecation code in a separate commit

In this case it seems like the deprecation may need to be done in Base’s bootstrap code.

juliohm · 2018-07-04T05:31:01Z

The build is passing without the deprecation warning. Can you advise on how to re-add the deprecation in Base's bootstrap code?

juliohm · 2018-07-05T20:10:51Z

Or if you prefer to merge and add the deprecation warning on a separate PR, that works too.

juliohm · 2018-07-10T16:40:39Z

Are you still interested in this change? The PR is ready.

ararslan

Should not be merged without a deprecation

StefanKarpinski · 2018-07-10T21:13:09Z

That deprecation was trickier than expected. Although values that are set on startup always are.

juliohm · 2018-07-11T05:05:35Z

Thanks for the addition, the build is passing, should be ready to merge 👍

StefanKarpinski · 2018-07-11T18:12:29Z

Resolved the conflict, letting CI run again, we can merge as soon as anything passes.

StefanKarpinski · 2018-07-11T18:12:42Z

Wrong button, sorry.

deprecations added

JeffBezanson · 2018-07-11T22:56:02Z

I agree we should eventually have CPU_LOGICAL_CORES and CPU_PHYSICAL_CORES. But, I also think it's ok to keep CPU_CORES as it is now (as an alias for CPU_LOGICAL_CORES), and just document that its meaning may change. For example, Stefan suggested that we might want to set CPU_CORES to logical or physical based on the architecture, basically picking a default for you.

juliohm · 2018-07-12T00:21:00Z

I don't know if that improves the situation. Picking defaults is always a tricky task, and the name CPU_CORES does not suggest something that is platform-dependent.

Besides, the chance of users adopting this platform-dependent default when they should have been explicit about what they wanted are big. Perhaps another name that is not CPU_CORES could be used for this platform-dependent default? Sys.MAX_THREADS?

JeffBezanson · 2018-07-12T03:42:01Z

I would be ok with making CPU_CORES refer to physical cores, and using MAX_THREADS for fake cores.

juliohm · 2018-07-12T03:51:38Z

So the proposal is to rename the current CPU_CORES to MAX_THREADS and leave the name CPU_CORES free for future implementation?

StefanKarpinski · 2018-07-12T13:20:50Z

CPU_CORES does not suggest something that is platform-dependent.

No one is suggesting that it be platform-dependent, the suggestion is that it be hardware-dependent... and it seems fairly obvious that CPU_CORES would depend on your hardware.

juliohm · 2018-07-12T14:19:20Z

Could you please clarify? Suppose I have a laptop with two physical cores that supports hyperthreading and can do four logical cores without performance hits. If I install Linux versus Windows, will CPU_CORES always return four and hence mean logical cores as initially discussed in this issue? My main concern is the name of the variable, which is inconsistent with other software conventions (e.g. hwloc).

…

On Thu, Jul 12, 2018, 6:21 AM Stefan Karpinski ***@***.***> wrote: CPU_CORES does not suggest something that is platform-dependent. No one is suggesting that it be platform-dependent. The suggestion is that it be hardware-dependent... and it seems fairly obvious that CPU_CORES would depend on your hardware. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMLbd2Ug5IAsCATqehQmSwBq7J127Beks5uF01IgaJpZM4U8ZKy> .

JeffBezanson · 2018-07-12T15:23:05Z

that supports hyperthreading and can do four logical cores without performance hits

Does such a thing exist?

If I install Linux versus Windows

No, as Stefan said this would be hardware-dependent, not OS-dependent.

I just don't want writing CPU_LOGICAL_CORES to be my only option for getting a core count.

juliohm · 2018-07-12T15:30:40Z

Sorry for the naive question, isn't hyperthreading technology implemented at the software level?

JeffBezanson · 2018-07-12T15:38:31Z

Not entirely; it also requires a hardware feature that exposes a different number of CPUs to the OS.

juliohm · 2018-07-12T15:51:39Z

So it is not only hardware-dependent right? That is what is concerning, the name CPU_CORES is very unclear. Most software use it for physical cores, a hardware quality.

…

On Thu, Jul 12, 2018, 8:38 AM Jeff Bezanson ***@***.***> wrote: Not entirely; it also requires a hardware feature that exposes a different number of CPUs to the OS. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMLbQb4D9oMFdpxCMgZk0abdrJ-OJ8Aks5uF22OgaJpZM4U8ZKy> .

JeffBezanson · 2018-07-12T16:05:51Z

No, the number of physical cores and hyper-threaded cores are both determined entirely by the hardware. The number of hyper-threads supported by your CPU does not change when you change your OS.

StefanKarpinski · 2018-07-12T17:15:05Z

The trouble is that naively people expect CPU_CORES to tell them how many cores there are and how many threads they should use. Both of those questions are subtler than they used to be—there is no one correct answer to either—and they may not be the same. You seem to have gotten burned by one interpretation, but there's no reason that the other meaning won't burn you just as badly.

juliohm · 2018-07-12T17:30:20Z

So maybe MAX_THREADS or other less confusing name would save future burns? It is natural to interpret CPU_CORES as cpu cores, which is not what the Julia implementation is doing.

…

On Thu, Jul 12, 2018, 10:15 AM Stefan Karpinski ***@***.***> wrote: The trouble is that naively people expect CPU_CORES to tell them how many "cores" there are and how many threads they should use. Both of those questions are subtler than they used to be and may not be the same. You seem to have gotten burned by one meaning, but there's no reason that the other meaning won't burn you just as badly. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMLbUfd38VMNbWqCEf_pPRvsDHOZCP8ks5uF4Q2gaJpZM4U8ZKy> .

StefanKarpinski · 2018-07-12T18:03:42Z

It is natural to interpret CPU_CORES as cpu cores, which is not what the Julia implementation is doing.

The point is that it depends on what the definition of what a "cpu core" is—and this is not clear-cut. You want to change it to mean "physical cores" based on a a single experience on one piece of hardware with one specific workload for which having as many threads as there are logical cores happened to cause suboptimal performance. On the same hardware, there are other workloads where having as many threads as there are logical cores will lead to optimal performance and only having as there are physical cores will lead to highly suboptimal performance. On yet other hardware (e.g. Xeon Phi), it is impossible to get optimal performance if the number of threads is limited to the number of physical cores: you can only get full usage of the hardware with hyper-threading.

Jeff's proposal is (as I understand it) to leave CPU_CORES as is but document that may change in the future and is defined to give you some value between the number of logical cores and physical cores that we believe to be a pretty good guess at how many threads you want to use for CPU-intensive workloads on your hardware. The current definition as logical cores is good for some machines (Xeon Phi) but bad for other machines, so this should be improved. In addition, we can expose the number of logical and physical cores separately.

JeffBezanson · 2018-07-12T18:50:49Z

+1 for CPU_CORES and CPU_THREADS.

juliohm · 2018-07-12T19:00:58Z

I see. If both physical and logical cores are exported as well besides the guessed value proposed, that would be fine. I still think that the name CPU_CORES is inappropriate for the current implementation. IMO, ideally we would have the following semantics: CPU_CORES: optimal guess, hard to define CPU_PHYS_CORES: physical cores CPU_LOGIC_CORES: logical cores This PR is adopting the above definitions, but I could shorten the name further from CPU_LOGICAL_CORES to CPU_LOGIC_CORES What do you think?

…

On Thu, Jul 12, 2018, 11:04 AM Stefan Karpinski ***@***.***> wrote: It is natural to interpret CPU_CORES as cpu cores, which is not what the Julia implementation is doing. The point is that it depends on what the definition of what a "cpu core" is—and this is not clear-cut. You want to change it to mean "physical cores" based on a a single experience on one piece of hardware with one specific workload for which having as many threads as there are logical cores happened to cause suboptimal performance. On *the same* hardware, there are other workloads where having as many threads as there are logical cores will lead to optimal performance and only having as there are physical cores will lead to highly suboptimal performance. On yet other hardware (e.g. Xeon Phi <https://en.wikipedia.org/wiki/Xeon_Phi>), it is impossible to get optimal performance if the number of threads is limited to the number of physical cores: you can only get full usage of the hardware with hyper-threading. Jeff's proposal is (as I understand it) to leave CPU_CORES as is but document that may change in the future and is defined to give you some value between the number of logical cores and physical cores that we believe to be a pretty good guess at how many threads you want to use for CPU-intensive workloads on your hardware. The current definition as logical cores is good for some machines (Xeon Phi) but bad for other machines, so this should be improved. In addition, we can expose the number of logical and physical cores separately. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMLbfiHRYwFtuqHkU1eP3ofqDDAsSkQks5uF4-SgaJpZM4U8ZKy> .

juliohm · 2018-07-12T19:03:03Z

Oh I like CPU_CORES and CPU_THREADS +1 On Thu, Jul 12, 2018, 12:01 PM Júlio Hoffimann <[email protected]> wrote:

…

I see. If both physical and logical cores are exported as well besides the guessed value proposed, that would be fine. I still think that the name CPU_CORES is inappropriate for the current implementation. IMO, ideally we would have the following semantics: CPU_CORES: optimal guess, hard to define CPU_PHYS_CORES: physical cores CPU_LOGIC_CORES: logical cores This PR is adopting the above definitions, but I could shorten the name further from CPU_LOGICAL_CORES to CPU_LOGIC_CORES What do you think? On Thu, Jul 12, 2018, 11:04 AM Stefan Karpinski ***@***.***> wrote: > It is natural to interpret CPU_CORES as cpu cores, which is not what the > Julia implementation is doing. > > The point is that it depends on what the definition of what a "cpu core" > is—and this is not clear-cut. You want to change it to mean "physical > cores" based on a a single experience on one piece of hardware with one > specific workload for which having as many threads as there are logical > cores happened to cause suboptimal performance. On *the same* hardware, > there are other workloads where having as many threads as there are logical > cores will lead to optimal performance and only having as there are > physical cores will lead to highly suboptimal performance. On yet other > hardware (e.g. Xeon Phi <https://en.wikipedia.org/wiki/Xeon_Phi>), it is > impossible to get optimal performance if the number of threads is limited > to the number of physical cores: you can only get full usage of the > hardware with hyper-threading. > > Jeff's proposal is (as I understand it) to leave CPU_CORES as is but > document that may change in the future and is defined to give you some > value between the number of logical cores and physical cores that we > believe to be a pretty good guess at how many threads you want to use for > CPU-intensive workloads on your hardware. The current definition as logical > cores is good for some machines (Xeon Phi) but bad for other machines, so > this should be improved. In addition, we can expose the number of logical > and physical cores separately. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#27856 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ADMLbfiHRYwFtuqHkU1eP3ofqDDAsSkQks5uF4-SgaJpZM4U8ZKy> > . >

StefanKarpinski · 2018-07-12T19:05:55Z

Based on triage discussion, "CPU cores" is most commonly used to mean "physical cores" whereas "CPU threads" is often used to mean "logical cores"—i.e. number of hardware threads (Intel is not entirely consistent about this, but this is the trend). Accordingly, an alternate proposal is to rename CPU_CORES to CPU_THREADS now and potentially reintroduce CPU_CORES in the future to give the number of physical cores. It should be noted that this number may not be as useful as people believe it to be. The number of threads that a process should use is a different matter, but is generally some n between CPU_CORES ≤ n ≤ CPU_THREADS. I'll update the PR accordingly.

juliohm · 2018-07-12T19:12:25Z

Awesome, I can adjust the PR by the end of the day, feel free to make the changes earlier as well...

…

On Thu, Jul 12, 2018, 12:06 PM Stefan Karpinski ***@***.***> wrote: Based on triage discussion, "CPU cores" is most commonly used to mean "physical cores" whereas "CPU threads" is often used to mean "logical cores"—i.e. number of hardware threads (Intel is not entirely consistent about this, but this is the trend). Accordingly, an alternate proposal is to rename CPU_CORES to CPU_THREADS now and potentially reintroduce CPU_CORES in the future to give the number of physical cores. It should be noted that this number may not be as useful as people believe it to be. The number of threads that a process should use is a different matter, but is generally some n between CPU_CORES ≤ n ≤ CPU_THREADS. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMLbZfZr9yi3a44LTaipz74TL9WQqzmks5uF54rgaJpZM4U8ZKy> .

StefanKarpinski · 2018-07-12T21:12:41Z

Got into some weird situation with GitHub remotes and it was easier to make a new PR: #28091.

juliohm commented Jul 1, 2018

View reviewed changes

ararslan previously requested changes Jul 10, 2018

View reviewed changes

StefanKarpinski force-pushed the cpucores branch from a48bee3 to ca0a36b Compare July 10, 2018 21:11

Rename Sys.CPU_CORES to more explict Sys.CPU_LOGICAL_CORES

4ee4bea

StefanKarpinski force-pushed the cpucores branch from 3898ec7 to a578d4f Compare July 11, 2018 18:11

StefanKarpinski closed this Jul 11, 2018

StefanKarpinski reopened this Jul 11, 2018

Add deprecation for Sys.CPU_CORES => Sys.CPU_LOGICAL_CORES

018d726

StefanKarpinski force-pushed the cpucores branch from a578d4f to 018d726 Compare July 11, 2018 18:53

StefanKarpinski added the triage This should be discussed on a triage call label Jul 12, 2018

StefanKarpinski closed this Jul 12, 2018

StefanKarpinski removed the triage This should be discussed on a triage call label Jul 12, 2018

tkf mentioned this pull request Sep 28, 2019

BLAS threads should default to physical not logical core count? #33409

Open

DilumAluthge mentioned this pull request Jan 22, 2021

Sys.CPU_CORES was renamed in Julia 1.0 m-j-w/CpuId.jl#42

Closed

Rename Sys.CPU_CORES to more explict Sys.CPU_LOGICAL_CORES #27856

Rename Sys.CPU_CORES to more explict Sys.CPU_LOGICAL_CORES #27856

Conversation

juliohm commented Jun 29, 2018

ararslan commented Jun 29, 2018

juliohm commented Jun 29, 2018

StefanKarpinski commented Jun 29, 2018

nalimilan commented Jun 29, 2018

juliohm commented Jun 29, 2018

nalimilan commented Jun 29, 2018

juliohm commented Jun 29, 2018 • edited Loading

StefanKarpinski commented Jun 30, 2018

juliohm commented Jun 30, 2018 via email

juliohm commented Jul 1, 2018

fredrikekre commented Jul 1, 2018

juliohm Jul 1, 2018

Choose a reason for hiding this comment

fredrikekre Jul 2, 2018

Choose a reason for hiding this comment

juliohm commented Jul 2, 2018

KristofferC commented Jul 3, 2018

StefanKarpinski commented Jul 3, 2018

juliohm commented Jul 4, 2018

juliohm commented Jul 5, 2018

juliohm commented Jul 10, 2018

ararslan left a comment

Choose a reason for hiding this comment

StefanKarpinski commented Jul 10, 2018

juliohm commented Jul 11, 2018

StefanKarpinski commented Jul 11, 2018

StefanKarpinski commented Jul 11, 2018

JeffBezanson commented Jul 11, 2018

juliohm commented Jul 12, 2018

JeffBezanson commented Jul 12, 2018

juliohm commented Jul 12, 2018

StefanKarpinski commented Jul 12, 2018 • edited Loading

juliohm commented Jul 12, 2018 via email

JeffBezanson commented Jul 12, 2018 • edited Loading

juliohm commented Jul 12, 2018

JeffBezanson commented Jul 12, 2018

juliohm commented Jul 12, 2018 via email

JeffBezanson commented Jul 12, 2018

StefanKarpinski commented Jul 12, 2018 • edited Loading

juliohm commented Jul 12, 2018 via email

StefanKarpinski commented Jul 12, 2018

JeffBezanson commented Jul 12, 2018

juliohm commented Jul 12, 2018 via email

juliohm commented Jul 12, 2018 via email

StefanKarpinski commented Jul 12, 2018 • edited Loading

juliohm commented Jul 12, 2018 via email

StefanKarpinski commented Jul 12, 2018

juliohm commented Jun 29, 2018 •

edited

Loading

StefanKarpinski commented Jul 12, 2018 •

edited

Loading

JeffBezanson commented Jul 12, 2018 •

edited

Loading

StefanKarpinski commented Jul 12, 2018 •

edited

Loading

StefanKarpinski commented Jul 12, 2018 •

edited

Loading