Memprof: provide the guarantee that an allocation callback is always run in the same thread the allocation takes place. Attempt II. by jhjourdan · Pull Request #9674 · ocaml/ocaml

jhjourdan · 2020-06-13T11:26:05Z

This PR is a second attempt at implementing the feature proposed by #9449.

In this new version, I use a separate extensible array for each thread, which contains the entries for the tracked blocks for which the allocation callback has not yet been called. Another shared array contains the entries for which the allocation callback has been called.

In addition, this PR contains a few code improvements which are related to this new data structure. In particular, tracking of young blocks has been made much simpler, since we know for sure that the allocating thread is the only one allowed to modify the local entry array (and hence no concurrent modification can happen when handling the different allocations of the same combined block).

This PR also gets rid of the idx_ptr pointers, which had relatively complex invariants. I am no completely sure that the replacement is better, though. Since this is in a separate commit, please tell me if we should of not use this.

Taking advantage of the removal of idx_ptr, this PR also lifts the restriction about Thread.exit in Memprof callbacks. Same remark as above: this is in a separate optional commit.

The diff of this PR is quite large, since it changes significantly the main data structure of Memprof. I tried to make independent commits to ease reviewing (only one commit should be non-trivial). Please review this one commit at a time.

Since this PR have more thread-local data structures, it should help rebasing on top of multicore. I'm happy to help.

Cc @stedolan, @gasche, @ctk21.

jhjourdan · 2020-06-13T11:29:06Z

One last comment: the first commit of this PR is actually #9653, which I'd loved to be merged first.

jhjourdan · 2020-06-25T16:17:16Z

@stedolan : any chance you could tell me what you think about this? Of course, you don't need to do a thorough review, but I'd appreciate to know your opinion.

jhjourdan · 2020-07-21T19:33:06Z

Would it be possible to get a review for this PR?

I know the diff is large, but I tried hard to split it into several commits, and this PR simplify some aspects of the implementation of Memprof.

I'd really like to see it merged for 4.12, and given that the diff is large, the review process may take some time. For the records, the first version of this PR, #9449, has been there since mid April, and did not receive much attention since then..

xavierleroy · 2020-09-03T09:22:39Z

@stedolan would you be available for reviewing this PR? I'm afraid it will just drop into oblivion otherwise.

jhjourdan · 2020-09-30T09:25:32Z

@stedolan, anychance you will have some time to have a look at this before 4.12?

stedolan · 2020-09-30T16:02:09Z

Yes! Apologies for the slowness, it's on my list but I haven't found time yet. I'll have a look early next week.

jhjourdan · 2020-09-30T16:11:00Z

Thanks !

And sorry if you feel a bit harassed : I am just afraid we won't have time before 4.12!

otherlibs/systhreads/st_stubs.c

runtime/memprof.c

stedolan · 2020-10-06T09:50:39Z

This looks good! I'm very glad to see the back of idx_ptr. I'm going to have a more careful read of the reentrancy logic now, though, because I think that remains the trickiest part.

By the way, is there still a test of what happens when Thread.exit is called during a callback?

runtime/memprof.c

stedolan · 2020-10-06T12:56:47Z

While reading this I thought of a slightly simplification: since each struct tracked can only be executing a callback in one thread at a time, we can make the struct tracked point to the thread's caml_memprof_th_ctx, so that when updating indices in flush_deleted we don't need the callback/closure/iteration setup.

This is a bit easier to explain in code than text. See stedolan@25563fb

jhjourdan · 2020-10-09T21:40:38Z

While reading this I thought of a slightly simplification: since each struct tracked can only be executing a callback in one thread at a time, we can make the struct tracked point to the thread's caml_memprof_th_ctx, so that when updating indices in flush_deleted we don't need the callback/closure/iteration setup.

This is a bit easier to explain in code than text. See stedolan/ocaml@25563fb

Thanks for the suggestion. I merged your ideas in the relevant commits of the PR.

stedolan · 2020-10-12T13:45:18Z

Thanks! I've written two more tests for the bits of this PR that I find trickiest. (see https://github.com/stedolan/ocaml/commits/memprof_thread_alloc_callback_2) One of them uncovers a segfault if we stop Memprof during a Memprof callback, mind having a look?

jhjourdan · 2020-10-15T11:59:13Z

I think you missed one comment: the test here still causes a segfault.

Thanks. I have now fixed this.

The only issue with this PR is that the new test moved_while_blocking.ml fails with flambda because of #9978...

xavierleroy · 2020-10-15T12:14:56Z

While the Flambda strangeness is being investigated, you can turn the test off in Flambda mode:

(* TEST
* hassysthreads
** no-flambda
include systhreads
*** bytecode
*** native
*)

jhjourdan · 2020-10-15T12:19:13Z

While the Flambda strangeness is being investigated, you can turn the test off in Flambda mode:

As you prefer. But I did not want to disable a test which reveals an actual bug! Actually it depends on whether #9978 can be investigated within short time bounds.

jhjourdan · 2020-10-15T13:25:00Z

Thanks to the suggestion of @lthls, I fixed the test. CI should pass soon.

runtime/memprof.c

gasche · 2020-10-19T14:12:37Z

The PR was approved and the CI is green. Should we merge the PR, or does @jhjourdan want to change the commit history?

(I looked at the history, my impression is that squashing everything would be not-nice, in particular it would make bisecting an issue within the PR much harder. There is a small amount of back-and-forth in the history, but I think it would be acceptable to merge right away if @jhjourdan doesn't happen to be a git history control addict.)

jhjourdan · 2020-10-19T14:33:48Z

Ok, let me have a look at history then.

This is only effetive in native mode, since function calls in bytecode mode will trigger polling and hence run pending callbacks.

…rray struct, which only contains information specific to the array.

…'s data structures. Reasons: - Better abstraction in memprof.h - Simpler Saving/restore functions - We introduce a current's thread context, so that we don't need to do a spacial case for the current thread

…e returned by the callback.

…run in the same thread the allocation takes place. This is done by using a local entry array for each thread, containing tracked blocks whose allocation callback has not yet been called. This allows some simplification in the code running callbacks for young allocations. Indeed, since the entry array is local to one thread, we know for sure that it cannot be modified during a callback, and therefore we no longer need to remember the indices of the corresponding new entries.

Instead, we use a thread-local variable [callback_status] which contains the index of the corresponding entry when a callback is running. We can do this since there can only be one running callback at the same time in a given thread. This lifts the restriction forbidding the call of Thread.exit from a memprof callback.

…ets killed by fork.

jhjourdan · 2020-10-19T15:13:26Z

Alright, the history looks better now, I think. This is ready to merge and cherry-pick to 4.12. (I don't have the commit bit, though.)

Memprof: provide the guarantee that an allocation callback is always run in the same thread the allocation takes place. Attempt II. (cherry picked from commit 0cb298f)

jhjourdan · 2020-10-20T08:54:39Z

Cherry-picked in 4.12 branch as part of commit 2c85e04.

jhjourdan mentioned this pull request Jun 13, 2020

Memprof: provide the guarantee that an allocation callback is always run in the same thread the allocation takes place. #9449

Closed

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 41b78e8 to 2171097 Compare June 15, 2020 13:27

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 2171097 to 905874a Compare June 25, 2020 15:40

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 15012b0 to 4af10f3 Compare June 26, 2020 10:10

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 4af10f3 to c43efd3 Compare July 6, 2020 06:13

jhjourdan mentioned this pull request Jul 28, 2020

EINTR-based signals, again #9722

Merged

1 task

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from c43efd3 to bd8073a Compare July 28, 2020 15:18

xavierleroy requested a review from stedolan September 3, 2020 09:21

stedolan self-assigned this Oct 5, 2020

stedolan requested changes Oct 6, 2020

View reviewed changes

otherlibs/systhreads/st_stubs.c Show resolved Hide resolved

runtime/memprof.c Outdated Show resolved Hide resolved

runtime/memprof.c Outdated Show resolved Hide resolved

stedolan reviewed Oct 6, 2020

View reviewed changes

runtime/memprof.c Show resolved Hide resolved

stedolan reviewed Oct 6, 2020

View reviewed changes

runtime/memprof.c Outdated Show resolved Hide resolved

stedolan reviewed Oct 6, 2020

View reviewed changes

runtime/memprof.c Show resolved Hide resolved

xavierleroy added this to the 4.12 milestone Oct 7, 2020

gadmm mentioned this pull request Oct 7, 2020

Purge memprof queue at program exit. #9198

Closed

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch 3 times, most recently from 4418ffd to 5372a4a Compare October 8, 2020 14:28

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 5372a4a to 38c46e2 Compare October 9, 2020 22:15

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 2b87c38 to 6d34733 Compare October 15, 2020 11:54

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 6d34733 to 54e829f Compare October 15, 2020 13:24

stedolan reviewed Oct 19, 2020

View reviewed changes

runtime/memprof.c Show resolved Hide resolved

stedolan approved these changes Oct 19, 2020

View reviewed changes

jhjourdan and others added 12 commits October 19, 2020 16:36

Whitespace, typos.

cad494b

Memprof: fix rounding error.

b371c21

Gc.Memprof.stop does no longer try to run pending callbacks.

d180082

This is only effetive in native mode, since function calls in bytecode mode will trigger polling and hence run pending callbacks.

Memprof: refactor realloc_trackst.

1141163

Memprof : get rid of the tracking_state struct, and create an entry_a…

13a874c

…rray struct, which only contains information specific to the array.

Memprof: thread-local context is now stored as a pointer in systhread…

ea52bec

…'s data structures. Reasons: - Better abstraction in memprof.h - Simpler Saving/restore functions - We introduce a current's thread context, so that we don't need to do a spacial case for the current thread

Memprof : Refactor [run_callback_exn] so that it also stores the valu…

18c0f95

…e returned by the callback.

Memprof/systhreads : call [caml_memprof_delete_th_ctx] when threads g…

b9abf83

…ets killed by fork.

Changes.

7e4748c

Add a test for Memprof.stop within a Memprof callback

a6980b2

jhjourdan force-pushed the memprof_thread_alloc_callback_2 branch from 54e829f to a6980b2 Compare October 19, 2020 15:12

gasche added the merge-me label Oct 19, 2020

jhjourdan merged commit 0cb298f into ocaml:trunk Oct 20, 2020

This was referenced May 21, 2021

4.12 - Windows: Crash in caml_memprof_handle_postponed_exn #10422

Closed

fix(sdl2): Remove some unsafe acquire/release calls revery-ui/revery#1072

Merged

Conversation

jhjourdan commented Jun 13, 2020

Uh oh!

jhjourdan commented Jun 13, 2020

Uh oh!

jhjourdan commented Jun 25, 2020

Uh oh!

jhjourdan commented Jul 21, 2020

Uh oh!

xavierleroy commented Sep 3, 2020

Uh oh!

jhjourdan commented Sep 30, 2020

Uh oh!

stedolan commented Sep 30, 2020

Uh oh!

jhjourdan commented Sep 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stedolan commented Oct 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stedolan commented Oct 6, 2020

Uh oh!

jhjourdan commented Oct 9, 2020

Uh oh!

stedolan commented Oct 12, 2020

Uh oh!

jhjourdan commented Oct 15, 2020

Uh oh!

xavierleroy commented Oct 15, 2020

Uh oh!

jhjourdan commented Oct 15, 2020

Uh oh!

jhjourdan commented Oct 15, 2020

Uh oh!

Uh oh!

gasche commented Oct 19, 2020

Uh oh!

jhjourdan commented Oct 19, 2020

Uh oh!

jhjourdan commented Oct 19, 2020

Uh oh!

jhjourdan commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants