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

Bigarray: do not change GC pace when creating subarrays or slices #12493

Closed
wants to merge 2 commits into from

Conversation

gasche
Copy link
Member

@gasche gasche commented Aug 23, 2023

This is a proposed fix for #12491.

The issue

When creating bigarrays, we increase the GC pacing in a way that should be proportional to the amount of "external" memory held by the bigarray. The bug in #12491 is that creating proxies (copies, slices) of an existing bigarray increases the GC speed as if new external memory was allocated, which is unnecessary (and hurts performance) as the proxy reuses memory from an existing bigarray.

The fix (details)

Before this PR, the logic was as follows:

  • caml_ba_alloc takes flags as input and data
  • if data is NULL, malloc some memory for the bigarray and add the MANAGED flag to flags
  • if flags has MANAGED, increase GC pacing on bigarray creation

After this PR, the logic is as follows:

  • if data is NULL, set a must_alloc boolean
  • if must_alloc, malloc some memory
  • if must_alloc, increase GC pacing on bigarray creation

The only change in behavior corresponds to calls that initially have the MANAGED flag in flags, but a non-null data field. Those would increase the GC pacing before the PR, and they do not increase it after the PR.

This exactly covers the case of slices of subarrays, but also possibly some other cases of caml_ba_alloc calls in user code.
I believe that the change is correct (a bugfix) in all these cases: if the caller of caml_ba_alloc passes existing memory and, at the same time, claims that this memory is owned by the OCaml heap, then we should not increase the GC pacing.

@gasche
Copy link
Member Author

gasche commented Aug 23, 2023

I believe that the change is correct (a bugfix) in all these cases: if the caller of caml_ba_alloc passes existing memory and, at the same time, claims that this memory is owned by the OCaml heap, then we should not increase the GC pacing.

After thinking more about this, I think that I was wrong to assume that passing the MANAGED flag means that "the memory is (already) owned by the OCaml heap". The user may wish instead to transfer ownership of new memory they just allocated to the OCaml heap. In this case the proposed bugfix is not quite right, and I prefer to close and discuss this further in #12491.

@xavierleroy
Copy link
Contributor

I was wrong to assume that passing the MANAGED flag means that "the memory is (already) owned by the OCaml heap". The user may wish instead to transfer ownership of new memory they just allocated to the OCaml heap.

It's not just a wish: the user gives the OCaml GC the right to free this new memory whenever it sees fit; that's very much an ownership transfer.

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