-
Notifications
You must be signed in to change notification settings - Fork 89
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
use specific implementation to get available host memory on MacOS #2597
base: develop
Are you sure you want to change the base?
Conversation
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.
The code now compiles without issues on my Mac. Thanks for fixing this!
|
||
return vmstat.free_count * pagesize; | ||
#else | ||
GEOS_ERROR_IF( percent > 100, "Error, percentage of memory should be smallerer than -100, check lifoOnHost (should be greater that -100)" ); |
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.
Copied here by accident?
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.
Indeed.
// remove these definitions from mach/boolean.h that can conflict with GEOS code (eg. InputFlags::FALSE) | ||
#undef TRUE | ||
#undef FALSE |
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.
Could you instead encapsulate this implementation and all system-specific includes in a separate translation unit (.cpp
)? I think LvArray/system.[h/c]pp
would be a good place to expose this functionality, if you can spare the effort to open an LvArray PR.
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'll change that, I tend to avoid commiting to LvArray as it is another project but another translation unit would be cleaner.
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.
* @brief Retieves current available memory on host | ||
* @return the available memory in bytes. | ||
*/ | ||
static inline size_t getAvailableMemory() |
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.
Maybe indicate in the function name that this is for host/system memory?
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.
Good idea.
This fixes a compilation error on MacOS where the method to obtain the current memory available on host doesn't not build because _SC_AVPHYS_PAGES is not available.