-
Notifications
You must be signed in to change notification settings - Fork 371
Bugfix and Improve cache parsing on newer CPUs #5193
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
base: master
Are you sure you want to change the base?
Conversation
|
Unfortunately, I am about to start an extended leave on Friday, and there is really no way I will be able to review this before then. Can someone comment on where these constants are actually used? I did not immediately find it via |
titodalcanton
left a comment
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.
No time to look at the functionality change here, but please revert the changes to the logging messages in pycbc/opt.py, and do not add commented-out code.
|
Thanks for pointing this out @vaishakp . @rahuldhurkunde also encountered a similar issue building a singularity image recently, so I think we do need to resolve this. As @josh-willis says though, a lot of this is legacy code that can probably be removed. SO, the only place where these things (and I note, only the pycbc/pycbc/filter/simd_correlate.py Line 59 in d791555
and here: pycbc/pycbc/events/threshold_cpu.py Line 33 in d791555
both of these are providing segmentation settings being passed onto the SIMD "threshold and cluster" and "correlate" C code that @josh-willis wrote. Now, the code that Josh wrote for this is beyond me, but its a lot faster than alternatives that I tried to implement (including scipy/numpy functions for the same things) when we moved from weave to cython .... It's why these functions were then ported "in place" with me adding a Cython interface, rather than replacing code with Cython code as I did almost everywhere else. There are some issues with this though: When this was written, with So I think I'm in favour of removing these values from PyCBC entirely, and just using default values (which are provided in both cases). @ahnitz thoughts? |
Summary
On newer CPUs (e.g. Intel 258v), the linux
getconffunction in glibc has issues fetching the correct cache configuration.It returns incorrect cache sizes for all caches, and returns nothing for cache associations.
This PR changes the way cache configuration is read in by pycbc.
Details
The incorrect cache reporting by
getconfledpycbcto throw the following error:lscpuprovided by (util-linux) gives accurate information about the caches. It is also included in MacOS.This PR changes the way cache configuration is read in by pycbc.
Previous plan:
New plan:
Bugfixes
Improvements