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

lesderid · 2019-07-22T11:30:36Z

This PR adds a shared pointer type __rcptr!T that does reference counting.

It's an alternative to __RefCount (#2679) in the sense that __RefCount would almost always be used to manage a malloc/free-managed block of memory.

__rcptr!T improves upon __RefCount by not exposing an thread-unsafe isUnique and by being simpler to use (e.g. default construction is allowed).

For simplicity, in this PR the counter is external to the memory managed by __rcptr, but it should be possible to modify __rcptr to use intrusive reference counting.

Most of the other issues from __RefCount do still persist. We're casting const/immutable away, but we hope to solve this through __mutable/__metadata. We also still have to use atomic increments/decrements due to implicit conversions to/from immutable.

wilzbach · 2019-07-22T11:44:19Z

src/core/experimental/rcptr.d

+    {
+        import core.memory : pureFree;
+
+        pureFree(ptr);


So this is a big problem here as we assume that ptr as allocated via malloc which is unfortunately not always the case. Maybe we can templatize __rcptr and allow the user to provide his own deallocate function? Alternatively, we would need to store a pointer to such a deallocation function in the count payload.

If we modify __rcptr to use intrusive ref counting we could properly support custom allocators (once we have them in druntime).

For now, should I just add a template parameter?

Sadly, I'm not sure how we'll be able to use custom allocators as we'll be hit by the same problem as in the past: calling allocate / deallocate on an immutable allocator instance.

The very first implementation that I tried (long ago) was with intrusive reference counting and custom allocators and that hit a lot of bumps down the road. This is why we've over-simplified the design, going from an array with intrusive RC and custom allocators support to the implementation of rcarray

This is why we've over-simplified the design, going from an array with intrusive RC and custom allocators support to the implementation of rcarray

I understand the troubles you face, but this solution kinda buys us nothing. At least, it buys me nothing personally.
I won't use not-invasive ref counting.

What are we trying to do with this solution? Is it a stop-gap, or is it a thing?

I won't use not-invasive ref counting.

Why not?

What are we trying to do with this solution?

Library ref-counting for RefCounting for rcarray & co . with some (future) compiler magic.

It would be incompatible with any of my existing code.

This isn't intended to be for users. Hence, __

What compiler magic are we envisioning?

I think it was planned to detect __RefCounted for better @safe safety, but it currently looks like Water is trying to generalize this with DIP 1021.

It would be incompatible with any of my existing code.

This isn't intended to be for users. Hence, __

I'm not sure what you mean... how do I make use of it then? What does it offer if not a platform specifying how ref-counting should function?

I want to know why opInc/opDec failed? After 10 years of waiting for this, this direction is embarrassingly feeble, and even slightly worse than C++, which we determined was so disappointing that we spent TEN YEARS trying to do something better. If we can't do something at least as good as C++, then common-sense suggests we just copy C++, at least then we'll be fully compatible with C++.

how do I make use of it then?

You don't.

What does it offer if not a platform specifying how ref-counting should function?

It's internal for now and the forseeable future as no one is fully sure on the direction.

I want to know why opInc/opDec failed?

I don't know.

this direction is embarrassingly feeble

We need rcarray now, but we don't want to expose the exact details of it yet. What's so wrong with that?

BTW I'm the wrong person to ask these questions to.

how do I make use of it then?

You don't.

Huh? Why are we introducing code that has no use?

What does it offer if not a platform specifying how ref-counting should function?

It's internal for now and the forseeable future as no one is fully sure on the direction.

Evidently.

I want to know why opInc/opDec failed?

I don't know.

I feel like we need to understand this very clearly.

this direction is embarrassingly feeble

We need rcarray now, but we don't want to expose the exact details of it yet. What's so wrong with that?

What do we need it for? We've done without it for a very long time...

BTW I'm the wrong person to ask these questions to.

Okay, but you're the only one posting on the thread that appears to have a sense of authority :)

Huh? Why are we introducing code that has no use?

It has a used. It will be immediately used for rcarray, but allowing us to not commit to the interface/design just yet.

What do we need it for? We've done without it for a very long time...

Sure, and I survived without a million dollars for a very long time too. However, we need to start somewhere if the vision of a fully @nogc D is ever to become a reality.

you're the only one posting on the thread that appears to have a sense of authority

Yeah that's pretty sad. Maybe this conversation is better moved to the NG?

src/core/experimental/rcptr.d

TurkeyMan · 2019-07-22T17:10:24Z

src/core/experimental/rcptr.d

+    alias CounterType = int;
+
+    private T* ptr = null;
+    private shared(CounterType)* count = null;


Still shared :(

Also not invasive; I will not use this. As the only person I'm aware of who has been harping on an on about RC endlessly since 2009, it would be a shame to see someone like me not use this.

TurkeyMan · 2019-07-22T17:16:09Z

src/core/experimental/rcptr.d

+        The pointer owned by this instance of `__rcptr`.
+    */
+    @system
+    inout(T*) get() inout


Pointer to payload must be scope. It must have a lifetime <= this.

src/core/experimental/rcptr.d

TurkeyMan · 2019-07-23T20:28:09Z

src/core/experimental/rcptr.d

+
+        // The counter is left at -1 when this was the last reference
+        // (i.e. the counter is 0-based, because we use calloc)
+        if (atomicOp!"=="(*count, 0) || atomicOp!"-="(*count, 1) == -1)


I don't know what atomicOp!"==" actually is, but I think this is not right either.
It should be a non-atomic comparison:

if (atomicLoad!(MemoryOrder.raw)(*count) == 0 || atomicOp!"-="(*count, 1) == -1)

The decrement should be relaxed too.

Looks like it should be right like this?

druntime/src/core/atomic.d

Lines 819 to 865 in 3c9b39f

{

// binary operators

//

// + - * / % ^^ &

// | ^ << >> >>> ~ in

// == != < <= > >=

static if ( op == "+" || op == "-" || op == "*" || op == "/" ||

op == "%" || op == "^^" || op == "&" || op == "|" ||

op == "^" || op == "<<" || op == ">>" || op == ">>>" ||

op == "~" || // skip "in"

op == "==" || op == "!=" || op == "<" || op == "<=" ||

op == ">" || op == ">=" )

{

TailShared!T get = atomicLoad!(MemoryOrder.raw)( val );

mixin( "return get " ~ op ~ " mod;" );

}

else

// assignment operators

//

// += -= *= /= %= ^^= &=

// |= ^= <<= >>= >>>= ~=

static if ( op == "+=" && __traits(isIntegral, T) && __traits(isIntegral, V1))

{

return cast(T)(atomicFetchAdd!(T)(val, mod) + mod);

}

else static if ( op == "-=" && __traits(isIntegral, T) && __traits(isIntegral, V1))

{

return cast(T)(atomicFetchSub!(T)(val, mod) - mod);

}

else static if ( op == "+=" || op == "-=" || op == "*=" || op == "/=" ||

op == "%=" || op == "^^=" || op == "&=" || op == "|=" ||

op == "^=" || op == "<<=" || op == ">>=" || op == ">>>=" ) // skip "~="

{

TailShared!T get, set;

do

{

get = set = atomicLoad!(MemoryOrder.raw)( val );

mixin( "set " ~ op ~ " mod;" );

} while ( !casByRef( val, get, set ) );

return set;

}

else

{

static assert( false, "Operation not supported." );

}

}

I'm not sure what or why you just linked to that? Or what you mean by 'like this'?

Oh , maybe I see what you're saying...
Considering this code:

TailShared!T get = atomicLoad!(MemoryOrder.raw)( val ); mixin( "return get " ~ op ~ " mod;" );

It's clear that the comparison is not atomic; despite the name of the function...

So yeah you're right, but while that may be true, I would recommend NOT calling a function called atomicOp that performs a non-atomic op... it's misleading at best, and for my money, ugly and harder to understand.

So yeah you're right, but while that may be true, I would recommend NOT calling a function called atomicOp that performs a non-atomic op... it's misleading at best, and for my money, ugly and harder to understand.

That's fair, especially considering atomicOp's documentation doesn't specify it.

It's really not a great library.

TurkeyMan · 2019-07-23T20:35:52Z

src/core/experimental/rcptr.d

+    }
+
+    ///
+    void opAssign(__rcptr!T rhs)


Now I'm really confused... you changed it to accept the arg by value, but then still ref-fiddling in the function?
All you need to do is swap the pointers. Taking the arg by value will increment the ref count, and then destruction of the local on return will decrement; you need to swap them before returning so it decrements the right object.

Sadly, this approach accepting the argument by-val does not optimise for the rhs.count == count case; it will do the ref fiddling in that case when it doesn't need to.

Ah, my bad, I see what you're saying now. It does get a little messy because there is no swap function in druntime and we have to swap both ptr and count.

It indeed doesn't optimise for the rhs.count == count case though, but I'm not sure how much of an issue that is?

Maybe we should have a swap function ;)

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

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

Add __rcptr!T

d3d5f5b

wilzbach reviewed Jul 22, 2019

View reviewed changes

thewilsonator reviewed Jul 22, 2019

View reviewed changes

src/core/experimental/rcptr.d Outdated Show resolved Hide resolved

lesderid mentioned this pull request Jul 22, 2019

Add core.experimental.refcount (continuation of #2608) #2679

Closed

Compare against 0 and use int instead of uint

3f7581a

TurkeyMan reviewed Jul 22, 2019

View reviewed changes

src/core/experimental/rcptr.d Show resolved Hide resolved

TurkeyMan reviewed Jul 22, 2019

View reviewed changes

src/core/experimental/rcptr.d Outdated Show resolved Hide resolved

lesderid added 2 commits July 23, 2019 13:35

Optimise delRef

756387f

Take rhs by value in opAssign

1add70c

TurkeyMan reviewed Jul 23, 2019

View reviewed changes

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

Open

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

RazvanN7 closed this Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lesderid commented Jul 22, 2019

wilzbach Jul 22, 2019 •

edited

Loading

lesderid Jul 22, 2019

edi33416 Jul 22, 2019

TurkeyMan Jul 22, 2019

wilzbach Aug 4, 2019

wilzbach Aug 4, 2019

TurkeyMan Aug 4, 2019 •

edited

Loading

wilzbach Aug 4, 2019

TurkeyMan Aug 4, 2019

wilzbach Aug 4, 2019

TurkeyMan Jul 22, 2019

TurkeyMan Jul 22, 2019 •

edited

Loading

TurkeyMan Jul 22, 2019 •

edited

Loading

TurkeyMan Jul 23, 2019

lesderid Aug 1, 2019 •

edited

Loading

TurkeyMan Aug 2, 2019

TurkeyMan Aug 2, 2019

lesderid Aug 2, 2019

TurkeyMan Aug 2, 2019

TurkeyMan Jul 23, 2019

lesderid Jul 31, 2019

TurkeyMan Jul 31, 2019

RazvanN7 commented Apr 28, 2022

	{
	// binary operators
	//
	// + - * / % ^^ &
	// \| ^ << >> >>> ~ in
	// == != < <= > >=
	static if ( op == "+" \|\| op == "-" \|\| op == "*" \|\| op == "/" \|\|
	op == "%" \|\| op == "^^" \|\| op == "&" \|\| op == "\|" \|\|
	op == "^" \|\| op == "<<" \|\| op == ">>" \|\| op == ">>>" \|\|
	op == "~" \|\| // skip "in"
	op == "==" \|\| op == "!=" \|\| op == "<" \|\| op == "<=" \|\|
	op == ">" \|\| op == ">=" )
	{
	TailShared!T get = atomicLoad!(MemoryOrder.raw)( val );
	mixin( "return get " ~ op ~ " mod;" );
	}
	else
	// assignment operators
	//
	// += -= *= /= %= ^^= &=
	// \|= ^= <<= >>= >>>= ~=
	static if ( op == "+=" && __traits(isIntegral, T) && __traits(isIntegral, V1))
	{
	return cast(T)(atomicFetchAdd!(T)(val, mod) + mod);
	}
	else static if ( op == "-=" && __traits(isIntegral, T) && __traits(isIntegral, V1))
	{
	return cast(T)(atomicFetchSub!(T)(val, mod) - mod);
	}
	else static if ( op == "+=" \|\| op == "-=" \|\| op == "*=" \|\| op == "/=" \|\|
	op == "%=" \|\| op == "^^=" \|\| op == "&=" \|\| op == "\|=" \|\|
	op == "^=" \|\| op == "<<=" \|\| op == ">>=" \|\| op == ">>>=" ) // skip "~="
	{
	TailShared!T get, set;

	do
	{
	get = set = atomicLoad!(MemoryOrder.raw)( val );
	mixin( "set " ~ op ~ " mod;" );
	} while ( !casByRef( val, get, set ) );
	return set;
	}
	else
	{
	static assert( false, "Operation not supported." );
	}
	}

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

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

Conversation

lesderid commented Jul 22, 2019

wilzbach Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan Aug 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

TurkeyMan Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesderid Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented Apr 28, 2022

wilzbach Jul 22, 2019 •

edited

Loading

TurkeyMan Aug 4, 2019 •

edited

Loading

TurkeyMan Jul 22, 2019 •

edited

Loading

TurkeyMan Jul 22, 2019 •

edited

Loading

lesderid Aug 1, 2019 •

edited

Loading