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

Fix minor heap stats for bigarrays allocated with custom data pointers. #10788

Conversation

toots
Copy link
Contributor

@toots toots commented Nov 22, 2021

While debugging a user-reported memory leak in liquidsoap here: savonet/liquidsoap#2054, after going down the rabbit hole, I ended up looking at this piece of code:

  data = aligned_alloc(alignment, len);
  if (data == NULL)
    uerror("aligned_alloc", Nothing);
  ans = caml_ba_alloc_dims(CAML_BA_MANAGED | CAML_BA_C_LAYOUT | CAML_BA_UINT8,
                           1, data, len);

I was able to confirm that, if data was passed as NULL here, the compiler would allocate the memory itself and no memory leak would be happening. Furthermore, allocating data using malloc like the compiler does also lead to a memleak. I was also able to confirm that, by forcing a Gc.full_major() on each allocation, no memory leak was happening.

After more investigations and head scratching, it turned out that the problem comes from this code:

  size = 0;
  if (data == NULL) {
    num_elts = 1;
    for (i = 0; i < num_dims; i++) {
      if (caml_umul_overflow(num_elts, dimcopy[i], &num_elts))
        caml_raise_out_of_memory();
    }
    if (caml_umul_overflow(num_elts,
                           caml_ba_element_size[flags & CAML_BA_KIND_MASK],
                           &size))
      caml_raise_out_of_memory();
    data = malloc(size);
    if (data == NULL && size != 0) caml_raise_out_of_memory();
    flags |= CAML_BA_MANAGED;
  }
  asize = SIZEOF_BA_ARRAY + num_dims * sizeof(intnat);
  res = caml_alloc_custom_mem(&caml_ba_ops, asize, size);

The size variable in this code is passed as 0 when using a custom data pointer, thus confusing the Gc about how much data the allocated value actually represents.

The natural fix seems to be to make sure that size is always filled.

@toots toots force-pushed the fix-minor-heap-for-bigarray-with-custom-data-pointer branch 5 times, most recently from 1592325 to 8203de4 Compare November 22, 2021 18:01
@toots toots changed the title Fix minor head stats for bigarrays allocated with custom data pointers. Fix minor heap stats for bigarrays allocated with custom data pointers. Nov 22, 2021
@toots toots force-pushed the fix-minor-heap-for-bigarray-with-custom-data-pointer branch from 8203de4 to c01c022 Compare November 22, 2021 18:03
@toots toots force-pushed the fix-minor-heap-for-bigarray-with-custom-data-pointer branch from c01c022 to a57220a Compare November 22, 2021 19:18
@damiendoligez
Copy link
Member

This is almost exactly the same patch as #11022, but this PR doesn't take the CAML_BA_MANAGED flag into account.

@toots toots deleted the fix-minor-heap-for-bigarray-with-custom-data-pointer branch January 23, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants