fix(systemMemorySizeMb): Improve system memory detection on BSD systems #60489
Conversation
- Added support for detecting total system memory on FreeBSD, OpenBSD, and NetBSD using `sysctl`. - Updated `systemMemorySizeMb()` to use `unsigned long` instead of `int` for better accuracy, especially on systems with large amounts of RAM. - Introduced a `constexpr unsigned long megabyte` to improve readability and avoid redundant calculations. - Unified memory detection logic for BSD systems, using `HW_PHYSMEM64` when available for better precision. Fixes incorrect memory reporting on BSD platforms where it previously returned `-1`.
455c9dd to
3e8ade5
Compare
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
| #elif defined(Q_OS_NETBSD) | ||
| #elif defined(Q_OS_FREEBSD) || defined(Q_OS_OPENBSD) || defined(Q_OS_NETBSD) | ||
| unsigned long physmem; | ||
| size_t size = sizeof( physmem ); |
There was a problem hiding this comment.
On NetBSD, sysctl(7) says that hw.physmem64 is a "64-bit integer'. That type would be int64_t, not unsigned long.
And hy.physmem is a "32-bit integer", so "int32_t".
| // For FreeBSD | ||
| int mib[2] = {CTL_HW, HW_PHYSMEM}; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
On FreeBSD, sysctl(2) will be called with size=8, but it wants 4. On a big-endian machine, this will get the wrong answer. I realize this is messy, but it seems best to ask for oldp/oldsize as int32_t/sizeof(int32_t) or int64_t/sizeof(int64_t) and not try to alias types. I'd just move the sysctl into the if.
| @@ -475,7 +475,7 @@ class CORE_EXPORT QgsApplication : public QApplication | |||
| * | |||
| * \since QGIS 3.26 | |||
| */ | |||
| static int systemMemorySizeMb(); | |||
| static unsigned long systemMemorySizeMb(); | |||
|
|
|||
There was a problem hiding this comment.
This is fixing the problem that "int", assuming int is at least 32 bits (qgis won't build on a PDP-11 last I checked :-), can only represent 2^31 * 2*20 = 2^51 bytes of RAM? Which is 2T? That seems fair, as such RAM is now merely expensive and hard to find a motherboard for. I would lean to using uint64_t, but that's aesthetics. I might add a comment that a 32-bit signed variable could only represent memory < 2T.
| @@ -475,7 +475,7 @@ class CORE_EXPORT QgsApplication : public QApplication | |||
| * | |||
| * \since QGIS 3.26 | |||
| */ | |||
| static int systemMemorySizeMb(); | |||
| static unsigned long systemMemorySizeMb(); | |||
There was a problem hiding this comment.
Is this exposed in a shared library?
| @@ -102,7 +102,7 @@ QgsImageCache::QgsImageCache( QObject *parent ) | |||
| } | |||
| else | |||
| { | |||
| const int sysMemory = QgsApplication::systemMemorySizeMb(); | |||
| const unsigned long sysMemory = QgsApplication::systemMemorySizeMb(); | |||
| if ( sysMemory > 0 ) | |||
| { | |||
| if ( sysMemory >= 32000 ) // 32 gb RAM (or more) = 500mb cache size | |||
There was a problem hiding this comment.
Probably doesn't belong in this PR but the units are wrong. MB, not mb which is millibits :-)
Also, the comment and the code don't match, as 32 GB of ram is 32768 MB.
Perhaps more seriously, this is checking for >= 32 GB, but a machine with 32 GB will have physmem64 being less, presumably due to memory stolen by the BIOS for frame bufffer, or the kernel.
On NetBSD 10 amd64 with 32 GB of actual RAM and no virtualization involved.
$ sysctl hw.physmem; sysctl hw.physmem64
hw.physmem = -1
hw.physmem64 = 34159534080
which is 32577.07 MB, 190.9 MB short of 32G. That matches the kernel's available memory from boot:
total memory = 32577 MB
avail memory = 31474 MB
So checking for "32GB" probably should be "> 30 GB".
|
I get what's going on, but it's not clear how to test that the fix is working (assuming I try the commit on 3.34.x). |
|
@lbartoletti If you are looking for a GDAL contribution, we have a CPLGetPhysicalRAM() in port/cpl_vsisimple.cpp . As GDAL is a required dependency of QGIS, it could perhaps make sense for QGIS to delegate to GDAL's CPLGetPhysicalRAM() ? GDAL's version has quite a lot of tuning on Linux to take into account the cgroup mechanism used by containers to further restrict actual usable RAM. |
|
on OpenBSD: works for me, thanks :) and if that can go in GDAL, that's even better ! |
Description
sysctl.systemMemorySizeMb()to useunsigned longinstead ofintfor better accuracy, especially on systems with large amounts of RAM.constexpr unsigned long megabyteto improve readability and avoid redundant calculations.HW_PHYSMEM64when available for better precision.Fixes incorrect memory reporting on BSD platforms where it previously returned
-1.cc @landryb @gdt