Memprof: provide the guarantee that an allocation callback is always run in the same thread the allocation takes place.#9449
Conversation
e36c8b6 to
657cc57
Compare
Perhaps I could say a bit more on that: first, this is something which have been discussed several times (including in #8920), and second, in a multithreaded setting, it makes little sense to try to handle the callbacks of the current thread while discarding the others. |
|
Over in #8920, when responding to @gadmm, I said:
@jhjourdan has since pointed out that this already happens for single-threaded programs, for the same reason. So, making this guarantee will not in fact make anything worse, and it would be nice to be able to rely on it. |
657cc57 to
1153486
Compare
gadmm
left a comment
There was a problem hiding this comment.
Here is for the first three commits. Overall they look good to me but I have found two issues in the form of questions. I do the other three separately since you have updated them since I started the review.
|
@jhjourdan How important is it to get this into 4.11? I won't be able to review it tomorrow, but I could have a look on Monday if it's urgent. (On the other hand, if you want this in 4.11 because you think the existing behaviour is broken, perhaps we could argue that this is a bugfix and spend a little longer reviewing it) |
|
(Personally I think that rushing to review a complex PR a couple days before a deadline is an anti-pattern of collaborative software development.) |
|
Would you like to submit the first patch separately to have it in 4.11? And some of the simple refactorings if you want, they do not look risky. |
1153486 to
6af745c
Compare
6af745c to
4c40586
Compare
07c83d3 to
438493e
Compare
438493e to
aa83f14
Compare
|
Now that #9458 is merged, I rebased on top of trunk. |
| if (trackst.young_idx == i) trackst.young_idx = j; | ||
| if (trackst.promote_dealloc_callback_idx == i) | ||
| trackst.promote_dealloc_callback_idx = j; | ||
| } |
There was a problem hiding this comment.
I don't see how the next_alloc_callback_idx fields are maintained when everything shuffles around here
There was a problem hiding this comment.
Indeed, this is completely bogus. This is not because of next_alloc_callback_idx, but rather because of the idx_ptr pointers which are not updated accordingly.
|
I've started reading this, but honestly I find all of the |
…nction [set_and_update_idx_ptr].
…run in the same thread the allocation takes place. This is done by maintaining a bag of pending allocation callbacks for each thread, in the form of an linked list.
This is only effetive in native mode, since function calls in bytecode mode will trigger polling and hence run pending callbacks.
|
Now that I look at what I have written again, I tend to agree with you :(. We should probably redesign the data structures. Fortunately, if we get something where each thread allocates in its own data structure, then this will be easier to port to Multicore with good efficiency. However, I know how much time we needed to spend on the current data structure, and I am afraid that this will take long too... Let me think more about this. |
aa83f14 to
0690ba1
Compare
|
Closing this PR in favor of #9674. |
This is done by maintaining a bag of pending allocation callbacks for each thread, in the form of an linked list.
We also change the behavior of
Memprof.stop, so that it does no longer try to run pending callbacks. This is only effective in native mode, since function calls in bytecode mode will trigger polling and hence run pending callbacks.This MR is rather large, but only one commit actually has non-trivial content. Reviewing should be easier by reading one commit after the other.