Clarified GC.addRoot/addRange behavior. #322

Merged
merged 2 commits into from Nov 1, 2012

Conversation

Projects
None yet
2 participants
Member

klickverbot commented Oct 13, 2012

This pull request is the result of the »RFC: Pinning interface for the GC« discussion on digitalmars.D and a resulting rather long IRC discussion between Alex and me.

It updates the core.memory docs to reflect my understanding of the GC.addRoot resp. addRange functions, which as far as I can see matches the actual implementation. Still, it might be a good idea for somebody else to have a second look at the changes.

klickverbot reopened this Oct 13, 2012

Member

alexrp commented Oct 19, 2012

Maybe also update removeRoot's docs just for good measure?

Member

klickverbot commented Oct 19, 2012

Yes, this would certainly be a good idea, and I already did that yesterday (as mentioned on IRC). Unfortunately, GitHub was offline when I had the changes ready, so I couldn't update the pull request. Will push the changes later today when I'm at home.

Member

alexrp commented Oct 19, 2012

OK, thanks!

@klickverbot klickverbot Reorder core.memory.GC methods to keep add/remove pairs together.
This commit merely swaps the two function definitions. addRoot and
addRange are really quite different semantically, there is no point
in mixing them.
22e0fdb
Member

klickverbot commented Oct 20, 2012

@alexrp: The promised updates.

@dsimcha, @complexmath: You might want to have a look at this.

@alexrp alexrp commented on an outdated diff Oct 20, 2012

src/core/memory.d
*
* Params:
- * p = A pointer to a valid memory address or to null.
+ * p = A pointer into a GC-managed memory block or null.
+ *
+ * Example:
+ * ---
+ * // Typical C-style callback mechanism; the passed function
+ * // is invoked with the user-supplied context pointer at a
+ * // later point.
+ * extern(C) void addCallback(void function(void*), void*);
+ *
+ * // Allocate an object on the GC heap (this would usually be
+ * // some application-specific context data.
@alexrp

alexrp Oct 20, 2012

Member

Missing closing paren.

@alexrp alexrp commented on an outdated diff Oct 20, 2012

src/core/memory.d
*
* Params:
- * p = A pointer to a valid memory address or to null.
+ * p = A pointer into a GC-managed memory block or null.
+ *
+ * Example:
+ * ---
+ * // Typical C-style callback mechanism; the passed function
+ * // is invoked with the user-supplied context pointer at a
+ * // later point.
+ * extern(C) void addCallback(void function(void*), void*);
+ *
+ * // Allocate an object on the GC heap (this would usually be
+ * // some application-specific context data.
+ * auto context = new Object;
+ *
+ * // Make sure that is not collected even if it is no longer

@alexrp alexrp commented on the diff Oct 20, 2012

src/core/memory.d
+ * // referenced from D code (stack, GC heap, …).
+ * GC.addRoot(cast(void*)context);
+ *
+ * // Also ensure that a moving collector does not relocate
+ * // the object.
+ * GC.setAttr(cast(void*)context, GC.BlkAttr.NO_MOVE);
+ *
+ * // Now context can be safely passed to the C library.
+ * addCallback(&myHandler, cast(void*)context);
+ *
+ * extern(C) void myHandler(void* ctx)
+ * {
+ * // Assuming that the callback is invoked only once, the
+ * // added root can be removed again now to allow the GC
+ * // to collect it later.
+ * GC.removeRoot(ctx);
@alexrp

alexrp Oct 20, 2012

Member

Remember to remove the NO_MOVE flag.

@alexrp

alexrp Oct 20, 2012

Member

To clarify: This is safe because any object reachable directly through roots (in this case the stack/registers) should be treated as NO_MOVE implicitly by a moving GC.

@klickverbot

klickverbot Oct 20, 2012

Member

I was not sure which way it would be less misleading, as using GC.clrAttr requires that you know that nobody else sets NO_MOVE – but I guess you are right, it should be included in the example to not cause confusion due to the asymmetry.

Member

klickverbot commented Oct 20, 2012

@alexrp: The »Notes to implementors« section should probably also be updated as it uses addRoot and addRange in one breath – is it really required that the whole GC memory block added by addRoot is scanned conservatively, or did you merely include it because you assumed that the common definition of »root« also applies here?

Member

alexrp commented Oct 20, 2012

From what I understand, addRoot currently scans the pointed-to object, too (in addition to considering it live), if it is a GC-managed block, yes?

Member

klickverbot commented Oct 20, 2012

Yes, it scans the whole block. But should we require conservative scanning? It might make sense for addRange, but I'm not so sure if it does here.

Member

alexrp commented Oct 20, 2012

Right, let's require that the GC scan the pointed-to block if it's managed by the GC, but remove the requirement that it has to be conservative.

Member

klickverbot commented Oct 20, 2012

The block pointed to by addRoot always has to be managed by the GC, otherwise addRoot is effectively a no-op. Does this need further clarification?

Member

alexrp commented Oct 20, 2012

Argh, no, the docs are clear enough about it. I'm just being silly.

Member

alexrp commented Oct 24, 2012

@klickverbot ping; waiting for your changes to the notes to implementors list.

Member

klickverbot commented Nov 1, 2012

As discussed with Alex on IRC, I will submit the changes to the documentation comment as a separate pull request as further, only tangentially related discussion might be needed on that.

alexrp merged commit c7383ee into dlang:master Nov 1, 2012

1 check was pending

default Pass: 7, Pending: 2
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment