Add core.experimental.refcount (continuation of #2608) by lesderid · Pull Request #2679 · dlang/druntime

lesderid · 2019-07-16T00:38:18Z

Continuation of #2608

This PR adds a struct __RefCount that can be used as a common building block for reference counted types through composition.

A short summary of the known issues that were brought up in the previous thread and earlier discussions:

The type needs to be prefixed with __ because the compiler might need to be made aware of its existence in the future (e.g. to inhibit elision of calls to const pure functions like addRef/delRef)
It's a non-intrusive approach, i.e. it doesn't use space lent by the object it's tracking, which means worse locality of reference and more (de)allocations
Because of implicit conversion of return values of pure functions to immutable (and immutable implying shared), we have to conservatively use atomic increments/decrements for the counter variable

None of these issues are bad enough to keep blocking progress. Early inclusion in core.experimental allows us to experiment with reference counted containers using this reference counting scheme (e.g. rcarray!T, #2646) while not yet fully committing to a specific implementation or API.

Squashed commit of the following: commit 56e1140 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Thu Jun 13 15:05:36 2019 +0300 Remove dead code commit b7b8a02 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue Jun 4 15:46:32 2019 +0300 Use shared type for refcount commit 95bb942 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 28 17:06:49 2019 +0300 Use factory function; remove std import commit 30cc23b Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 28 16:45:20 2019 +0300 Add threading test commit 21474ed Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 28 14:45:34 2019 +0300 Add support for shared qualifier commit 3fa96c2 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 18:37:26 2019 +0300 Rename to __RefCount commit edf7c31 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 17:05:57 2019 +0300 Add ddoc commit 7a91e19 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 14:35:34 2019 +0300 Fix style commit 9359780 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 14:33:24 2019 +0300 isUnique is false if uninitialized commit ec62500 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 14:25:28 2019 +0300 Remove superfluous const attr from ctor commit 02c04c9 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 14:21:05 2019 +0300 Refactor test function commit 3a7e157 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Tue May 21 14:00:41 2019 +0300 Rename to _RefCount and place at topfile commit d77baa2 Author: Eduard Staniloiu <edi33416@gmail.com> Date: Mon May 20 18:50:04 2019 +0300 Mark opAssign as return; return this commit fbaf79e Author: Eduard Staniloiu <edi33416@gmail.com> Date: Wed May 15 19:41:49 2019 +0300 Refactor commit 5c2883f Author: Eduard Staniloiu <edi33416@gmail.com> Date: Wed May 15 19:14:05 2019 +0300 Add RefCount struct

The functionality being tested is already covered by unit tests.

dlang-bot · 2019-07-16T00:38:20Z

Thanks for your pull request and interest in making D better, @lesderid! 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 coverage diff by visiting the details link of the codecov check)
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#2679"

thewilsonator · 2019-07-16T00:47:06Z

src/core/experimental/refcount.d

+        // function is implicitly convertible to `immutable`.
+
+        shared CounterType* support = cast(shared CounterType*) pureAllocate(CounterType.sizeof);
+        *support = 1; // Start with 1 to avoid calling an `addRef` from 0 to 1


This will fail to compile with the new (not yet implemented) restricted shared, please do `*cast()support = 1;.

I changed shared CounterType* to shared(CounterType)*, as it's the counter that should be shared, not the pointer to it (as @thewilsonator pointed out in private). This makes this line compile with -preview=restrictiveshared (dlang/dmd#10142), but that seems like a bug in the restrictive shared implementation to me?

I think so, probably best to change it anyway.

This makes this line compile with -preview=restrictiveshared

No it doesn't... shared(int) can't be assigned.

Perhaps *cast(CounterType*)support = 1?

We should really have cast(ref CounterType)lvalue to perform lvalue casting...

wilzbach · 2019-07-16T10:18:08Z

src/core/experimental/refcount.d

+
+Implementation: The internal implementation of `__RefCount` uses `malloc`/`free`.
+
+Important: The `__RefCount` member must be initialized through a call to its


Can't we just disable the default initialization to enforce this?

@disable this();

This breaks rcarray for some reason: src/core/experimental/array.d(676): Error: variable `core.experimental.array.rcarray!int.rcarray.opSlice!(immutable(rcarray!int)).opSlice.__copytmp2437` default construction is disabled for type immutable(__RefCount)

Sorry, where is src/core/experimental/array.d? somewhere in your fork of druntime?

Yes, it's here, sorry: #2646

src/core/experimental/refcount.d

wilzbach · 2019-07-16T10:37:57Z

src/core/experimental/refcount.d

+        // a2 is the last ref to rcarray(4242) -> gets freed
+    }
+    assert(a.rc.isUnique);
+}


Can we add a few more test to increase the coverage of this module and ensure it works as expected?
Maybe even one with multiple threads to ensure the atomic ops work too.

Can we add a few more test to increase the coverage of this module and ensure it works as expected?

A bunch of tests were deleted with this commit

Maybe even one with multiple threads to ensure the atomic ops work too.

There was one which got deleted as well.

wilzbach · 2019-07-16T10:38:37Z

src/core/experimental/refcount.d

+
+License: $(HTTP www.boost.org/LICENSE_1_0.txt, Boost License 1.0).
+
+Authors: Eduard Staniloiu


@lesderid please feel free to add your name here.

wilzbach · 2019-07-16T10:42:35Z

src/core/experimental/refcount.d

+    private shared CounterType* rc = null;
+
+    /*
+    Perform `rc op val` operation. Always use atomics as the counter is shared.


Hmm, I guess this is the most controversial part of this PR as this will make it very slow for single-threaded programs.
Can't we separate the overloads, s.t. this is only done when __RefCount is actually shared (and thus its shared functions are called?).
Alternatively, maybe we need to add a __TLSRefCount?

Sadly, we can't. With implicit conversion of immutable(__RefCount) to const(__RefCount) we can't rely on this being shared.

The solution @edi33416 was trying out for this used alignment as a marker for __RefCounts that were constructed as shared (e.g. immutable), so after implicit conversion to const we can still know it was initially immutable.

However this doesn't work either, as the result of pure functions can be implicitly converted to immutable. This means that when creating and returning a __RefCount from a pure function, the non-shared constructor ends up being called for a shared object, meaning it gets constructed with non-shared alignment, and we're right back at square one.

I have a whole lot of problems with this conversation. It seems like there are mistakes everywhere.

@TurkeyMan Any insights you might have about this particular issue would be very much appreciated.

I haven't had time to think on it sorry, as funny as it might seem to some, RC is not actually an urgent issue for me as it has been for the last 10 years >_<
This is very important, but I need to devote 100% attention to shared right now.

My instinct is that RC is so important, that immutable->shared conversion causing us an issue here is satisfactory grounds to re-consider immutable->shared conversion rules... the problem might not be here, maybe it's there. (I don't know, I haven't thought on it at all, that's just my feeling)

I've had the same thought. I think it's definitely worth looking into.

TurkeyMan · 2019-07-19T06:27:13Z

src/core/experimental/refcount.d

+unittest
+{
+    auto rc = __RefCount.make();
+    assert(rc.isUnique);


You talk about atomic rc, but then there's isUnique? They're incompatible.

TurkeyMan · 2019-07-19T06:32:47Z

src/core/experimental/refcount.d

+    import core.atomic : atomicOp;
+
+    alias CounterType = uint;
+    private shared(CounterType)* rc = null;


No offense personally, but this is the most disappointing implementation of reference counting that is possible to write. We can do better than this, and if we can't, then we have serious language issues.

TurkeyMan · 2019-07-19T06:37:54Z

src/core/experimental/refcount.d

+    @nogc nothrow pure @trusted scope
+    private shared(CounterType) rcOp(this Q, string op)(CounterType val) const
+    {
+        return cast(shared(CounterType)) (atomicOp!op(*(cast(shared(CounterType)*) rc), val));


How does this work? rc is const, yet you mutate it... D's const is transitive, and D doesn't have mutable or anything like it, and the optimiser is free to do whatever it likes with the knowledge a value is const.
I don't think this line can go in the core runtime...?

Also, where's the memory order argument? It looks like it would execute this seq-cst (assuming that's the default), which is wrong.

TurkeyMan · 2019-07-19T06:42:14Z

src/core/experimental/refcount.d

+    This increases the reference count.
+    */
+    @nogc nothrow pure @safe scope
+    this(return scope ref typeof(this) rhs)


Why typeof(this) and not __RefCount?

It's idiomatic D to not repeat the struct name and reference it via typeof.

Where's that written? I've seen Walter recommending against typeof(this) used in this way on numerous occasions.

In Phobos source code.

Not a good place to find great D code ;)

TurkeyMan · 2019-07-19T06:42:25Z

src/core/experimental/refcount.d

+        rc = cast(typeof(rc)) support;
+    }
+
+    private enum copyCtorIncRef = q{


Please don't do this, it's not debug-able.

What would be the alternative?

Does a mixin template work here? If not, just repeat the code; its only 3 lines.
Why does it need to be repeated so many times anyway? Can inout be used to reduce duplication?

It does use inout?

Oh yeah, sorry I didn't see it in the sea of attributes. I was expecting it attributed to the context.
Can a constructor be inout? Wouldn't that cut the number of ctors down to 2; one inout and one inout shared?

TurkeyMan · 2019-07-19T06:43:36Z

src/core/experimental/refcount.d

+
+    private enum copyCtorIncRef = q{
+        rc = rhs.rc;
+        assert(rc == rhs.rc);


What kind of failure is this assert trying to catch?

TurkeyMan · 2019-07-19T07:05:17Z

An interesting observation to me, is that this seems like a whole lot more code (maybe 3-4 times as much code) as the same functionality would be in C++, and that's even considering that we're using those ugly text mixins everywhere to reduce the code bloat... I think it's possible that D is still lacking necessary foundation for this :/

Since this solution is functionally identical to C++, we should expect it to take the same amount of lines, or less.

TurkeyMan · 2019-07-19T07:06:46Z

src/core/experimental/refcount.d

+         $(BIGOH 1).
+    */
+    pure nothrow @safe @nogc scope
+    bool isUnique(this Q)() const


This function must be removed. I can't imagine anything you could do with this that's not a data race.

Removing the call to isInitialized will remove the data race at the cost of generating a segfault for an invalid (uninitialized) __RefCounted instance.

This function provides a simple interface towards the user to help him decide if he needs to deallocate the internal state of his data structure.
An "alternative" to this would be to expose the internal delRef to the user, which could break / invalidate the automatic reference counting. The user should never be able to explicitly modify the RC, but only "view" the value.

I propose that we bite the bullet and take the segfault.

If the RC is atomic, then none of those things you say can exist. Everything you just said is a data race.
If RC is thread-local (as it should be in D), then we have options.

UplinkCoder · 2019-07-19T16:18:23Z

I can't review this properly right now. Please hold off on merging until I had a proper look. Which I'll signal by approving this pr.

lesderid · 2019-07-19T16:28:21Z

Yes, this shouldn't be merged until we've been able to address the issues Manu pointed out.

(Having a feature to turn PRs into draft PRs would be very useful for cases like this, so it could be helpful to give that suggestion some support here: https://github.bokerqi.topmunity/t5/H/F/m-p/19107)

lesderid · 2019-07-22T12:22:33Z

We're trying out a possible alternative, a shared pointer type: #2690.

anon17 · 2019-07-24T09:39:14Z

You can have local counter, if you share immutable data in a shared container that would provide local rc wrappers for threads. When counter reaches zero you can see if it came from a shared container. See also this.

RazvanN7 · 2022-04-28T15:25:32Z

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

edi33416 and others added 4 commits July 16, 2019 01:51

Clean up ddoc to adhere to D Style

f6eb707

Remove extra tests

30d5022

The functionality being tested is already covered by unit tests.

Add top-level example

1cabdf2

lesderid requested review from andralex and wilzbach as code owners July 16, 2019 00:38

lesderid changed the title ~~Add core.experimental.__RefCount (continuation of #2608)~~ Add core.experimental.refcount (continuation of #2608) Jul 16, 2019

thewilsonator reviewed Jul 16, 2019

View reviewed changes

wilzbach reviewed Jul 16, 2019

View reviewed changes

lesderid added 6 commits July 18, 2019 15:10

shared CounterType* => shared(CounterType)*

cd06ebb

Remove unused testing code

38d8ecf

Optimise opAssign and use mixin for deduplication

664cb27

Clean up assert messages

654bf7e

Use inout for const copy constructors

c169401

Make constructor private and detemplatise make()

43ccdf8

thewilsonator mentioned this pull request Jul 19, 2019

Preliminary shared work dlang/dmd#10142

Closed

TurkeyMan reviewed Jul 19, 2019

View reviewed changes

lesderid mentioned this pull request Jul 22, 2019

Add __rcptr!T (possible alternative to #2679) #2690

Closed

lesderid mentioned this pull request Aug 26, 2019

Add core.experimental.rc #2760

Closed

wilzbach mentioned this pull request Dec 18, 2019

Are we @nogc yet? dlang/project-ideas#56

Closed

dlang-bot added Needs Rebase needs a `git rebase` performed stalled labels May 27, 2021

RazvanN7 closed this Apr 28, 2022


		Implementation: The internal implementation of `__RefCount` uses `malloc`/`free`.

		Important: The `__RefCount` member must be initialized through a call to its


		License: $(HTTP www.boost.org/LICENSE_1_0.txt, Boost License 1.0).

		Authors: Eduard Staniloiu

Uh oh!

Comments

Conversation

lesderid commented Jul 16, 2019

Uh oh!

dlang-bot commented Jul 16, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurkeyMan Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesderid Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TurkeyMan Jul 19, 2019 •

edited

Loading

lesderid Jul 18, 2019 •

edited

Loading

TurkeyMan Jul 19, 2019 •

edited

Loading

TurkeyMan commented Jul 19, 2019 •

edited

Loading