Don't pin objects permanently in engine.cpp

[d-netto]

Creates a side table of objects pinned by inference engine.

Adds/removes objects to the table at the start/end of inference.

[d-netto]

Should fix mmtk/mmtk-julia#246.

[d-netto]

Sharing some fragmentation time-series for the inference benchmark, similar to those we discussed on Slack:

• Stock GC:

• PR (e19c0da):

[udesou]

I still wonder whether pin/unpin is the right approach here - i.e., whether we could actually move those objects.
Could we instead:
(1) potentially traverse these objects and add the references from InferKey as roots - we might want to use something similar but perhaps slightly different than the vm specific roots in

julia/src/gc-mmtk.c

Line 769 in fb84d8f

JL_DLLEXPORT void jl_gc_scan_vm_specific_roots(RootsWorkClosure* closure)

otherwise they'll be pinned for every GC as well.
(2) adapt your code to capture the address of these objects into objects_pinned_by_inference_engine and give those objects to the GC as roots?

[d-netto]

Your suggestions might allow us to move more objects, but I'm not sure if they are necessary at the moment.

If we find that my approach introduces unnecessary pinning, then we can optimize it further.

But this PR is already a huge performance win, and seems "good enough" as it is.

[udesou]

Your suggestions might allow us to move more objects, but I'm not sure if they are necessary at the moment.

If we find that my approach introduces unnecessary pinning, then we can optimize it further.

But this PR is already a huge performance win, and seems "good enough" as it is.

I agree, but I don't see the need to pin/unpin these. While pinning/unpinning is the direction we should take for conservative stack pinning since we don't know whether those references are or are not actual roots, these are roots. A simple change is to remove the code for pinning/unpinning and actually process those roots in jl_gc_scan_vm_specific_roots:

for (size_t i = 0; i < gc_pinned_objects.len; i++) {
      void *obj = gc_pinned_objects.items[i];
      add_node_to_roots_buffer(closure, &buf, &len, obj);
}

You might want to rename the variable gc_pinned_objects etc.

[d-netto]

Wait, are these pinned_refs stored in the C++ data structures traced in the current implementation?

[udesou]

Wait, are these pinned_refs stored in the C++ data structures traced in the current implementation?

They are indefinitely pinned AFAIK. But that was just a conservative solution that we used for all the dominated roots that are not traced by the GC.

[d-netto]

Ok, so here is the reasoning behind my implementation choice of pinning them, but not rooting them:

• Current implementation permanently pins them, but doesn't register them as roots. So there must be other part of the code rooting them.
• This other part of the code is unchanged in my PR, and will still continue rooting them.

Am I missing something?

[udesou]

My suggestions (untested) according to the point I've made above: https://github.com/mmtk/julia/compare/dcn-optimize-pinning-in-inference-engine...udesou:udesou-minor-refactoring?expand=1.

Ok, so here is the reasoning behind my implementation choice of pinning them, but not rooting them:

• Current implementation permanently pins them, but doesn't register them as roots. So there must be other part of the code rooting them.
• This other part of the code is unchanged in my PR, and will still continue rooting them.

Am I missing something?

But that's why we had this problem in the first place right? (i.e., we have a root that is not being treated as a root because it's being kept alive by some other reference somewhere else)
And what we're trying to do is to make rooting explicit, at least in the long run, I'm not saying that this should be necessarily the case in this PR alone. Note that pinning does not make them roots. It works as a conservative solution (a "hack") and as Yi mentioned above, that macro serves as documentation to let us know the places where this happens. The solution above should not only work the same way but it also does the "right" thing, it treats those objects as roots. Furthermore it simplifies your code removing the need to pin/unpin those objects.

[qinsoon]

Ok, so here is the reasoning behind my implementation choice of pinning them, but not rooting them:

* Current implementation permanently pins them, but doesn't register them as roots. So there must be other part of the code rooting them.

* This other part of the code is unchanged in my PR, and will still continue rooting them.

Am I missing something?

You can look at our talk (we shared a link on Slack), the second part that talks about moving GCs. We explained why the current 'rooting' in Julia is insufficient. The definition of 'rooting' in Julia is flawed.

[d-netto]

Yes, I understand what you are saying.

I agree we should root these objects in the C++ data structure at some point, and stop relying on them getting rooted by some other part of the code.

I believe that chasing the C++ data structures and other spots in the code that this may happen is a topic for another PR, though.

[qinsoon]

chasing the C++ data structures and other spots in the code that this may happen is a topic for another PR

Those places where we use jl_pinned_ref are the roots we identified. We use pinning just for simplicity. This PR removes an occurrence of jl_pinned_ref. Ideally, we could just trace it as a root node, rather than a different mechanism for pinning/unpinning for GC. But I think the current PR is fine. We can tidy it up later.

[udesou]

Sounds good to me.