Skip to content

Fix mallinfo use#3183

Open
abalabin-bamfunds wants to merge 1 commit into
perspective-dev:masterfrom
abalabin-bamfunds:fix-mallinfo
Open

Fix mallinfo use#3183
abalabin-bamfunds wants to merge 1 commit into
perspective-dev:masterfrom
abalabin-bamfunds:fix-mallinfo

Conversation

@abalabin-bamfunds
Copy link
Copy Markdown

fix the use of mallinfo function:

  • on Linux use mallinfo2 as mallinfo is deprecated and buggy as per https://www.man7.org/linux/man-pages/man3/mallinfo.3.html - the deprecation warning is gone from the build after the change
  • in wasm use mallinfo as there is no mallinfo2 implementation available; however work around the bug outlined on the same page above to do with the returned datastructure declaring fields as signed while in the implementation they are not hence producing 'wrap around zero' bugs - the sysinfo call can now report use of more than 2GB of memory by wasm (this is probably not worth building a unit test around though)

Signed-off-by: Alexander Balabin <abalabin@bamfunds.com>
Copy link
Copy Markdown
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good!

I should have waited for CI to finish before reviewing this!

@texodus texodus changed the title fix mallinfo use Fix mallinfo use Jun 3, 2026
Copy link
Copy Markdown
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

This change fails to build in CI on GA's Ubuntu 22.04 x86_64 and aarch64 targets, likely due to the Manylinux docker environment these builds run in.

I will merge this if it can be gated to the correct glibc to pass CI, but this means it will not end up in generic wheel builds currently anyway, only when building Perspective from source.

@tomjakubowski
Copy link
Copy Markdown
Contributor

tomjakubowski commented Jun 3, 2026

To support both mallinfo and mallinfo2 in a single binary, maybe you could use dlsym() to call mallinfo2 when it's available, and otherwise fall back to mallinfo. It would require inlining the definition of the mallinfo2 struct since the header won't define it for glibc < 2.33.

@abalabin-bamfunds
Copy link
Copy Markdown
Author

I guess there is no appetite for migrating to manylinux_2_34? I must admit I do not know what repercussions of that would be and quick googling suggests there are.

I'll have a look at loading mallinfo2 dynamically.

@texodus
Copy link
Copy Markdown
Member

texodus commented Jun 4, 2026

Adding support for manylinux_2_34 isn't a problem, but removing support for manylinux_2_28 means old linux systems cannot upgrade Perspective.

manylinux_2_28 can be deprecated iff @timkpaine says it is ok.

@timkpaine
Copy link
Copy Markdown
Member

nope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants