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

count bytes allocated through malloc more precisely #55223

Closed
wants to merge 1 commit into from

Conversation

d-netto
Copy link
Member

@d-netto d-netto commented Jul 23, 2024

Should make the accounting for memory allocated through malloc a bit more accurate.

Should also simplify the accounting code by eliminating the use of jl_gc_count_freed in jl_genericmemory_to_string.

Works fine on my M2, but let's wait for CI to see how portable this patch is.

@d-netto d-netto added the GC Garbage collector label Jul 23, 2024
@d-netto d-netto requested review from vtjnash and gbaraldi July 23, 2024 17:06
@vtjnash
Copy link
Member

vtjnash commented Jul 23, 2024

SGTM. Assuming we can build on musl, I don't see a problem with this being de facto portable to any future memory allocator we would use

@d-netto d-netto force-pushed the dcn-allocated-bytes-in-alloc branch 3 times, most recently from 9d0850b to 28c4fa4 Compare July 26, 2024 01:43
@d-netto d-netto force-pushed the dcn-allocated-bytes-in-alloc branch from 28c4fa4 to a6abd18 Compare August 5, 2024 15:03
@gbaraldi
Copy link
Member

gbaraldi commented Aug 5, 2024

I would just measure how expensive this call is on a hot path (I suspect it's super cheap so LGTM)

@d-netto
Copy link
Member Author

d-netto commented Aug 5, 2024

Latest version is still not ready for merging though.

Even though the latest commits passed on Linux looks like the macOS-aarch64 tests hang (despite building fine on it).

@d-netto d-netto force-pushed the dcn-allocated-bytes-in-alloc branch from a6abd18 to 03c1fab Compare August 5, 2024 17:43
@gbaraldi
Copy link
Member

gbaraldi commented Aug 5, 2024

One thing that looking at this made me see is the malloc_good_size function that macos has. I wonder if we should take it into account for the overallocation things we have.

@d-netto d-netto force-pushed the dcn-allocated-bytes-in-alloc branch 5 times, most recently from 0a40c6e to 005caee Compare August 12, 2024 16:10
@d-netto
Copy link
Member Author

d-netto commented Aug 12, 2024

On Windows:

C:/workdir/src/gc-common.c:122:12: error: implicit declaration of function '_aligned_msize'; did you mean '_aligned_free'? [-Wimplicit-function-declaration]
  122 |     return _aligned_msize(p, JL_CACHE_BYTE_ALIGNMENT, 0);
      |            ^~~~~~~~~~~~~~
      |            _aligned_free

Microsoft docs tell that this function should be in the <malloc.h> header: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/aligned-msize?view=msvc-170, so the failure seems confusing to me.

@d-netto d-netto force-pushed the dcn-allocated-bytes-in-alloc branch from 005caee to e0bd1bd Compare August 12, 2024 18:50
@d-netto
Copy link
Member Author

d-netto commented Sep 11, 2024

Not pursuing it further.

@d-netto d-netto closed this Sep 11, 2024
@giordano giordano deleted the dcn-allocated-bytes-in-alloc branch October 7, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants