Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Understanding rcarray better #320

Open
jmh530 opened this issue Nov 12, 2020 · 7 comments
Open

Understanding rcarray better #320

jmh530 opened this issue Nov 12, 2020 · 7 comments

Comments

@jmh530
Copy link
Contributor

jmh530 commented Nov 12, 2020

I'm a little confused by the behavior of the code below. Foo accepts some memory in payload, but then when I use the asRCArray function it seems as if a copy is being made.

Also, taking a slice of counts and passing it to Foo does not increment the counter, even though it seems as if there is a reference back to counts (in that changing counts results in changes in foo and not x).

I've also tried compiling this with -dip1000 and @safe: at the top.

struct Foo
{
    double[] payload;

    this(double[] x)
    {
        payload = x;
    }
    
    auto asRCArray()
    {
        import mir.rc.array: rcarray;
     	return payload.rcarray;
    }
}

void main() {
    import mir.rc.array: mininitRcarray;
    
    size_t len = 5;
    auto counts = len.mininitRcarray!double;
    size_t i = 0;
    while (i < len) {
    	counts[i] = i;
        i++;
    }
    assert(counts._counter == 1);
    Foo foo = Foo(counts[]);
    assert(counts._counter == 1); //why not 2?
    auto x = foo.asRCArray();
    counts[2] = 10;
    assert(foo.payload[2] == 10);
    //assert(x[2] == 10); //fails to compile
    assert(x._counter == 1);
    assert(counts._counter == 1); 
}
@9il
Copy link
Member

9il commented Nov 13, 2020

rcarray is a function that creates a new RCArray. The type of double RCArray is RCArray!double. RCArray!double.opIndex (the [] operator) returns a slice of the underlying memory.

DIP1000 isn't well implemented

@jmh530
Copy link
Contributor Author

jmh530 commented Nov 13, 2020

When you say rcarray creates a new RCArray you mean that it creates a copy, right?

I also would have thought that taking a slice with [] would increase the reference count. So for instance, I was surprised that the code below compiles. But if opIndex is just providing a copy and not a reference, then it makes sense.

void main() {
    import mir.rc.array: mininitRcarray;
    
    size_t len = 5;
    auto counts = len.mininitRcarray!double;
    assert(counts._counter == 1);
    auto x = counts;
    assert(counts._counter == 2);
    auto y = counts.asSlice;
    assert(counts._counter == 3);
    auto z = counts[];
    assert(counts._counter == 3);
}

When I modified the code from the first post as below, I was able to get some of the behavior that I was expecting, though it is not integrated into rcarray. Could this be used to give out a reference to the mir_rcarray._payload and still increment the reference counter?

struct Foo
{
    double[] payload;

    this(double[] x)
    {
        payload = x;
    }
    
    ref double[] getPayloadRef() return
    {
        import mir.rc.array: rcarray;
     	return payload;
    }
}

void main() {
    import mir.rc.array: mininitRcarray;
    
    size_t len = 5;
    auto counts = len.mininitRcarray!double;
    size_t i = 0;
    while (i < len) {
    	counts[i] = i;
        i++;
    }
    Foo foo = Foo(counts[]);
    auto x = foo.getPayloadRef();
    counts[2] = 10;
    assert(foo.payload[2] == 10);
    assert(x[2] == 10);
    assert(counts._counter == 1); 
}

@9il
Copy link
Member

9il commented Nov 13, 2020

When you say rcarray creates a new RCArray you mean that it creates a copy, right?

Yes

Could this be used to give out a reference to the mir_rcarray._payload and still increment the reference counter?

RCArray!T should be used instead.

import core.lifetime: move;
import mir.rc.array;

struct Foo
{
    RCArray!double payload;

    this(RCArray!double x)
    {
        payload = move(x);
    }
    
	double[] getPayloadRef() return scope
    {
        import mir.rc.array: rcarray;
     	return payload[];
    }
}

void main()
{    
    size_t len = 5;
    auto counts = len.mininitRcarray!double;
    size_t i = 0;
    while (i < len) {
    	counts[i] = i;
        i++;
    }
    Foo foo = Foo(counts);
    auto x = foo.getPayloadRef();
    counts[2] = 10;
    assert(foo.payload[2] == 10);
    assert(x[2] == 10);
    assert(counts._counter == 2); 
}

@jmh530
Copy link
Contributor Author

jmh530 commented Nov 13, 2020

@9il Thanks for the reply.

When I try to also allow it to work with a GC slice, then I get errors again.


import core.lifetime: move;
import mir.rc.array;

struct Foo
{
    RCArray!double payload;

    this(RCArray!double x)
    {
        payload = move(x);
    }
    
    this(double[] x)
    {
        payload = rcarray(x.move);
    }
    
    double[] getPayloadRef() return scope
    {
        import mir.rc.array: rcarray;
     	return payload[];
    }
}

void main()
{    
    size_t len = 5;

    double[] counts = [0, 1, 2, 3, 4];
    Foo foo = Foo(counts);
    auto x = foo.getPayloadRef();
    counts[2] = 10;
    assert(foo.payload[2] == 10); //fails
    assert(x[2] == 10); //fails
}

The particular use case I was thinking about is for my work on mir.stat's histogram, which I have picked back up work on recently. Where I came across this issue is that I have some initial HistogramAccumulator that stores count data and I want to be able to 1) allow it to work with different allocation strategies (RC/GC/custom) for the counts and 2) to be able to provide some kind of range interface (I was thinking this would be a separate struct that would be returned from a member function). I'm tempted to just make the count data always an RC array, but I haven't made up my mind.

@9il
Copy link
Member

9il commented Nov 13, 2020

maybe a GC will be good to start and then we can think about nogc api

@jmh530
Copy link
Contributor Author

jmh530 commented Nov 13, 2020

The current implementation is here (i.e. from before this discussion).

@9il
Copy link
Member

9il commented Nov 17, 2020

The current implementation is here (i.e. from before this discussion).

@jmh530
That is great work!

Looking forward to the PR. (WIP status PR LGTM)

Lets, don't care about an RC API for now. If we can provide a lower-level API that accepts user-provided memory, then it should be fine. A high-level API can GC-allocate memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants