Skip to content
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.

Sign up for GitHub

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

fix(systemMemorySizeMb): Improve system memory detection on BSD systems #60489

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(systemMemorySizeMb): Improve system memory detection on BSD systems #60489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion python/PyQt6/core/auto_generated/qgsapplication.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ Returns a string name of the operating system QGIS is running on.
.. seealso:: :py:func:`platform`
%End

static int systemMemorySizeMb();
static unsigned long systemMemorySizeMb();
%Docstring
Returns the size of the system memory (RAM) in megabytes.

Expand Down
2 changes: 1 addition & 1 deletion python/core/auto_generated/qgsapplication.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ Returns a string name of the operating system QGIS is running on.
.. seealso:: :py:func:`platform`
%End

static int systemMemorySizeMb();
static unsigned long systemMemorySizeMb();
%Docstring
Returns the size of the system memory (RAM) in megabytes.

Expand Down
33 changes: 25 additions & 8 deletions src/core/qgsapplication.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ const QgsSettingsEntryStringList *QgsApplication::settingsSearchPathsForSVG = ne
#include <sys/sysinfo.h>
#endif

#if defined(Q_OS_FREEBSD) || defined(Q_OS_OPENBSD) || defined(Q_OS_NETBSD)
#include <sys/types.h>
#include <sys/sysctl.h>
#endif

#define CONN_POOL_MAX_CONCURRENT_CONNS 4

QObject *ABISYM( QgsApplication::mFileOpenEventReceiver ) = nullptr;
Expand Down Expand Up @@ -1422,8 +1427,9 @@ QString QgsApplication::osName()
#endif
}

int QgsApplication::systemMemorySizeMb()
unsigned long QgsApplication::systemMemorySizeMb()
{
constexpr unsigned long megabyte = 1024 * 1024;
#if defined(Q_OS_ANDROID)
return -1;
#elif defined(Q_OS_MAC)
Expand All @@ -1434,22 +1440,33 @@ int QgsApplication::systemMemorySizeMb()
memoryStatus.dwLength = sizeof( MEMORYSTATUSEX );
if ( GlobalMemoryStatusEx( &memoryStatus ) )
{
return memoryStatus.ullTotalPhys / ( 1024 * 1024 );
return memoryStatus.ullTotalPhys / megabyte;
}
else
{
return -1;
}
#elif defined(Q_OS_LINUX)
constexpr int megabyte = 1024 * 1024;
struct sysinfo si;
sysinfo( &si );
return si.totalram / megabyte;
#elif defined(Q_OS_FREEBSD)
return -1;
#elif defined(Q_OS_OPENBSD)
return -1;
#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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

#ifdef HW_PHYSMEM64
// NetBSD and OpenBSD have HW_PHYSMEM and HW_PHYSMEM64
// Use 64 bit version
int mib[2] = {CTL_HW, HW_PHYSMEM64};
#else
// For FreeBSD
int mib[2] = {CTL_HW, HW_PHYSMEM};
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( sysctl( mib, 2, &physmem, &size, nullptr, 0 ) == 0 )
{
return physmem / megabyte;
}

return -1;
#elif defined(Q_OS_UNIX)
return -1;
Expand Down
2 changes: 1 addition & 1 deletion src/core/qgsapplication.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ class CORE_EXPORT QgsApplication : public QApplication
*
* \since QGIS 3.26
*/
static int systemMemorySizeMb();
static unsigned long systemMemorySizeMb();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this exposed in a shared library?


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* Returns the QGIS platform name, e.g., "desktop", "server", "qgis_process" or "external" (for external CLI scripts).
Expand Down
2 changes: 1 addition & 1 deletion src/core/qgsimagecache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Expand Down