Add __RefCount struct

[edi33416]

This PR adds the __RefCount struct which is designed to be composed inside any user defined type that desires reference counting.

A simple example of usage

struct TestRC {   
        private __RefCount rc; 
        int[] payload;

        this(int sz) {
                rc = __RefCount(1);
                payload = (cast(int*) malloc(sz * int.sizeof))[0 .. sz];
        }

        void opAssign(ref TestRC rhs) {
                if (rc.isUnique) free(payload.ptr);
                
                rc = rhs.rc;
                payload = rhs.payload;
        }

        /* Implement copy ctors */

        ~this() {
                if (rc.isUnique) free(payload.ptr);
        }
}

[dlang-bot]

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2608"

[rainers]

A couple of comments about the design would be nice.

What happens when structs like TestRC are cast between mutable/shared/immutable?

[n8sh]

Doesn't something like this really belong in Phobos?

[JinShil]

Where does this fit in the grander picture? I see the tree, but what role does it play in the ecosystem of the forest. Also, this appears to be a runtime lowering. Is there a compiler PR being planned so ref-counted objects are first-class citizens in the D language?

[edi33416]

Doesn't something like this really belong in Phobos?

The reason why this is placed in druntime is because achieving reference counting with all the attributes that the language provides, is a difficult problem and we want to make it available to all our users.

We also desire to use it in rcarray, rcstring and the like.

[edi33416]

Where does this fit in the grander picture? I see the tree, but what role does it play in the ecosystem of the forest. Also, this appears to be a runtime lowering. Is there a compiler PR being planned so ref-counted objects are first-class citizens in the D language?

Reference counting is a pressing issue in the D language and we want to have a very basic type that takes care of all the magic required to get reference counting to work with pure and immutable objects/functions.

There is no planned lowering, for now, that I am aware of.

[n8sh]

The reason why this is placed in druntime is because achieving reference counting with all the attributes that the language provides, is a difficult problem and we want to make it available to all our users.

That's not a great reason in my opinion. Lots of non-trivial stuff is in Phobos, and Phobos is available to all users and has similar things like std.typecons.RefCounted which works in betterC.

We also desire to use it in rcarray, rcstring and the like.

If those are going to be added to the D runtime then that would be a good reason. rcarray sounds a lot like the standard library's std.container.array, so I would similarly ask why that would be added to the D runtime instead of to the standard library.

[n8sh]

I have concerns about promoting the use of __RefCount as a building-block for other types. By necessity it allocates storage for the counter separately from whatever resource is associated with the __RefCount. In a handwritten reference-counted data structure the storage for the payload and the storage for the counter could be allocated together and deallocated together. Using this __RefCount would increase the number of allocations and decrease locality of reference, while not doing much to decrease effort of implementation (compared to for example std.typecons.RefCounted!T which in addition to incrementing/decrementing a counter also calls a destructor when the counter hits zero and has optional auto-initialization).

[lesderid]

Couldn't this return const(CounterType*)?

[atilaneves]

So no other allocators then? I know this is a druntime PR so it wouldn't be able to use std.experimental.allocator, but then I wonder why not put this in phobos.

[edi33416]

It's placed in druntime as it might require some compiler "help" in the future. For now, all the functions that are called for a dec ref return a void*, ex void* delRef() const. This is so in the context of an immutable object the pure const delRef call won't be elided. Currently there is no magic involved, but If, in the future, the compiler will see that the returned value is never used and it decides to elide, we might need a helping hand.

[thewilsonator]

call won't be elided.

Then I'd be making sure to thoroughly with LDC and GDC. Also in the case of dead object elimination (as opposed to just dead call) that is the desired outcome.

[edi33416]

Also in the case of dead object elimination (as opposed to just dead call) that is the desired outcome.

Good point. I'd assume that in this case, since the object is "dead" it will elide the calls regardless of any other considerations, as the calls are made through the object

[atilaneves]

Any such compiler help would require a DIP, no?

[edi33416]

Any such compiler help would require a DIP, no?

I believe so

[andralex]

So no other allocators then? I know this is a druntime PR so it wouldn't be able to use std.experimental.allocator, but then I wonder why not put this in phobos.

We wanted to get a minimal version going because we have these huge problems with purity and immutable etc. Worrying about custom memory allocation is additional distraction. Too many holes in the dam, too few fingers.

We must put this in druntime because that's where the refcounted slice needs to be (i.e. a slice that is just like T[] but manages its own memory).

[atilaneves]

I would take all of the code out of druntime, compile it with ldc and check with asan that there are no leaks or other memory problems.

[wilzbach]

I would take all of the code out of druntime, compile it with ldc and check with asan that there are no leaks or other memory problems.

Excellent point, but we should better setup a CI step/task for this as otherwise future additions and improvements won't be checked.

[atilaneves]

I would take all of the code out of druntime, compile it with ldc and check with asan that there are no leaks or other memory problems.

Excellent point, but we should better setup a CI step/task for this as otherwise future additions and improvements won't be checked.

Good point there too. I'm not sure how to set up compiling druntime itself with asan though, and only for ldc.

[wilzbach]

I'm not sure how to set up compiling druntime itself with asan though, and only for ldc.

Well, yet another of those many reasons why we should have LDC/LLVM as official backend. Anyhow, that not being a short-term option, the easiest way would be to not compile full druntime (because of it's compiler magic that might be problematic), but only compile the specific __RefCount test(s) with ldc and -fsanitize=address?
LDC also has its own runtime build too (see e.g. https://wiki.dlang.org/Building_LDC_runtime_libraries), but I'm not sure how well this will work with master (e.g. name mangling changes in master etc.).

In other words:

• use Buildkite (we could also use other CIs, but we want to unite all CIs to Buildkite) and setup a new target (LDC runtime) - the existing DMD druntime target is here
• install LDC (we typically use this script)
• invoke the respective test script with DMD=ldc2 (and thus let it add sanitization)

[9il]

This PR adds the __RefCount struct which is designed to be composed inside any user defined type that desires reference counting.

1. http://mir-algorithm.libmir.org/mir_rc_array.html
2. http://mir-algorithm.libmir.org/mir_rc_ptr.html

are based on the Mir's own RC context, which is more memory efficient and can be used with allocators. Please don't make the language and runtime depend on this type the way users would not be able to use their own type with the same experience. The docs shouldn't define it as a unique RC context for D, just a reference implementation.

[JinShil]

I still don't see how this fits into the larger picture, probably because this PR doesn't explain what the long-term reference counting plan is. Do users have to add this __RefCount field and supporting boiler-plate to their types any time they want to have reference-counted semantics? Or is there some future compiler integration planned where the boilerplate is added by the compiler? How is this different from Phobos's RefCounted type? Can a __RefCount field also be added to classes?

[andralex]

Doesn't something like this really belong in Phobos?

This is a core type virtually part of the language definition.

[andralex]

I have concerns about promoting the use of __RefCount as a building-block for other types. By necessity it allocates storage for the counter separately from whatever resource is associated with the __RefCount. In a handwritten reference-counted data structure the storage for the payload and the storage for the counter could be allocated together and deallocated together. Using this __RefCount would increase the number of allocations and decrease locality of reference, while not doing much to decrease effort of implementation (compared to for example std.typecons.RefCounted!T which in addition to incrementing/decrementing a counter also calls a destructor when the counter hits zero and has optional auto-initialization).

We don't have a better solution, and we don't even know very well how to get this to work (regardless of the intrusive aspect and orthogonal to it). In the future we may add an intrusive version a la __RefCountedIntrusive!(T, "member") to use space lent by the host object. This is from the "I have a dream" cycle.

[9il]

I have concerns about promoting the use of __RefCount as a building-block for other types. By necessity it allocates storage for the counter separately from whatever resource is associated with the __RefCount. In a handwritten reference-counted data structure the storage for the payload and the storage for the counter could be allocated together and deallocated together. Using this __RefCount would increase the number of allocations and decrease locality of reference, while not doing much to decrease effort of implementation (compared to for example std.typecons.RefCounted!T which in addition to incrementing/decrementing a counter also calls a destructor when the counter hits zero and has optional auto-initialization).

We don't have a better solution, and we don't even know very well how to get this to work (regardless of the intrusive aspect and orthogonal to it). In the future we may add an intrusive version a la __RefCountedIntrusive!(T, "member") to use space lent by the host object. This is from the "I have a dream" cycle.

It is cool that we would have RC support in DRuntime.

mir.rc uses intrusive RCs for pointers and arrays.

Please don't introduce a compiler/druntime changes that would optimize only DRuntime defined RC context. User-defined RC context implementation should have a clear way to get the optimizations as well, hope so.

[lesderid]

What's keeping this from getting merged? I realise that an intrusive approach might be preferable and it's a shame that we're currently stuck with using atomic ops for all instances (#2608 (comment)).

That said, this PR is targeting core.experimental and it's currently blocking progress on rcarray et al.

[JinShil]

I still don't see how this fits into the larger picture,

I understand this now after seeing #2646

[n8sh]

it's currently blocking progress on rcarray et al.

Perhaps this is a good candidate for using a development branch.

[wilzbach]

Perhaps this is a good candidate for using a development branch.

Unfortunately, they have never worked in the past. A core.experimental package is probably the only realistic chance we have.

[RazvanN7]

RC requires __mutable to be implemented. There is a reference to this PR here: https://issues.dlang.org/show_bug.cgi?id=23071