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.

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

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

XL64
Copy link
Contributor

@XL64 XL64 commented Jul 24, 2023

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.

@XL64 XL64 self-assigned this Jul 24, 2023
@XL64 XL64 added the type: bug Something isn't working label Jul 24, 2023
@XL64 XL64 requested review from rrsettgast and herve-gross July 24, 2023 14:50
Copy link
Contributor

@herve-gross herve-gross left a 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)" );
Copy link
Contributor

Choose a reason for hiding this comment

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

Copied here by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

Comment on lines 52 to 54
// remove these definitions from mach/boolean.h that can conflict with GEOS code (eg. InputFlags::FALSE)
#undef TRUE
#undef FALSE
Copy link
Contributor

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.

Copy link
Contributor Author

@XL64 XL64 Jul 25, 2023

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.

Copy link
Contributor Author

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()
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

@XL64 XL64 changed the title use specific implementation ti get available host memory on MacOS use specific implementation to get available host memory on MacOS Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants