Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

step one of moving .dup to library#758

Closed
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:dupeOfEarl
Closed

step one of moving .dup to library#758
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:dupeOfEarl

Conversation

@WalterBright
Copy link
Member

Looks like Andrei and I had the same idea:

dlang/dmd#3364

After this is pulled, the followup will be to remove it from the compiler. Good riddance!

And, in 2.068, remove the internal druntime function adDup(). Keep for the moment so git bisect will work.

@yebblies
Copy link
Contributor

So now if the postblit is not pure, we'll get a compile error from inside idup?

Keep for the moment so git bisect will work.

Except it won't, because we break abi compatibility every couple of weeks.

@WalterBright
Copy link
Member Author

So now if the postblit is not pure, we'll get a compile error from inside idup ?

Yes, if the postblit is impure, as it should anyway. It relies very much on attribute inference.

Except it won't, because we break abi compatibility every couple of weeks.

Hence I try not to.

@yebblies
Copy link
Contributor

Yes, if the postblit is impure, as it should anyway. It relies very much on attribute inference.

Then this will cause a diagnostic regression. Maybe add a static assert?

@WalterBright
Copy link
Member Author

Then this will cause a diagnostic regression. Maybe add a static assert?

Can you give an example? Currently it gives a just fine message:

Error: cannot implicitly convert expression (dup(s)) of type S[] to immutable(S)[]

and before it would still fail .idup if the members weren't implicitly convertible to immutable.

@monarchdodra
Copy link
Contributor

It will fail to compile for types with disabled T.init. Also, the const(T[]) input will prevent types with indirections from properly functioning (because !s(const(T) : T)).

struct S1
{
    @disable this();
}
struct S3
{
    int* p;
}
void main() pure
{
    S1[] s1;
    S3[] s3;
    idup(s1); //Fails
    idup(s3); //Fails
}

I think we could get it to work if we allowed a "fallback" to _adDupT for said types. We'd at least get type inference "more often" which is still a win.


Also, the code seems very inefficient, since it will T.init the memory, and then copy things over it. For types without elaborate postblit, it is un-necessary.

It gets trickier for types with postblit though...

@WalterBright
Copy link
Member Author

@monarchdodra s3.idup fails both with the current compiler and the proposed change, as it should.

As for efficiency, yes, it is somewhat slower because of the initialization, but not that slower. The allocation cost will still dominate unless a very large array is being duped. BTW, the semantics for postblit require that the destination be already initialized.

The new design also allows a number of things to work that did not work before, specifically, the test cases added to #3364

@WalterBright
Copy link
Member Author

BTW, I think the solution to this efficiency problem, and the disabled .init, is to improve operator new so it can take an array initializer.

@ghost
Copy link

ghost commented Mar 28, 2014

You mean things like:

auto arr = new int[](void);

Since recently we can do:

void main()
{
    auto intPtr = new int(10);
    assert(*intPtr == 10);
}

We can't yet do auto intPtr = new int(void);, but I think it would be a welcome addition, as would new int[](initializer).

@ghost
Copy link

ghost commented Mar 28, 2014

And I'm aware that we have a syntax for multidim initialization new int[][](2, 4), I'm not too sure how to handle this.

@MartinNowak
Copy link
Member

So can we do anything to avoid the initialisation?
Using GC.malloc directly would require to poke around in rt.lifetime internals. It might be worth the trouble. Still I'd like to see this getting merged soonish.

@monarchdodra
Copy link
Contributor

@monarchdodra s3.idup fails both with the current compiler and the proposed change, as it should.

My apologies, I pasted the wrong code. I meant with normal dup. Here is the correct code:

struct S
{
    int* p;
}

void main()
{
    S[] p;
    p.dup; //Passes
    dup(p); //Fails
}

BTW, the semantics for postblit require that the destination be already initialized.

Technically not true, depending on how define "postblit": Entire postblit "chain" consists in first bit-copying the source onto the destination, and then calling "this(this)". The initial value of destination is irrelevant.

It's only true if you are doing a "postblit-based assign", since it will call the destroyer on destination.

As for efficiency, yes, it is somewhat slower because of the initialization, but not that slower. The allocation cost will still dominate unless a very large array is being duped.

OK.

The new design also allows a number of things to work that did not work before, specifically, the test cases added to #3364

Yes, this is most awesome. IMO, even if slower, it's a definite gain. But performance asides, the current implementation is too simplistic, and will simply fail for certain types.

Still, I say if we can use the new function for "most" types, and fall back to the old one behavior for the rest, it's still a big win.

Also, may I request you also add a testcase in 3364 that does the opposite? That a type with unsafe, impure and throwing postblit, will also make dup the same?

@monarchdodra
Copy link
Contributor

It's only true if you are doing a "postblit-based assign", since it will call the destroyer on destination.

..Right, so initialization is needed since a[] = b[] cause "postblit assignment". Please ignore my comment then -_-.

@MartinNowak
Copy link
Member

How about this, we add an extern(C) void[] _alloc_array(size_t length, size_t elemSize) pure nothrow; function to rt lifetime. The function will return uninitialized memory. Then we can memcpy the data and possibly call postblit for each element.

@monarchdodra
Copy link
Contributor

Then we can memcpy the data and possibly call postblit for each element.

The root issue is we don't have any function that can call postblit, while preserving inference. typeid(T).postblit doesn't infer anything. We could write a template method that does it (it's been requested before), but writting it in druntime without std.typetuple nor std.traits would be very complicated. Doable, but complicated.

My vote goes to getting a simple working solution merged in, and then refine it from there.

Of course, types with postblit are the minority, so we could just implement your solution for those types, and deal with postblit types later...

@MartinNowak
Copy link
Member

.__postblit?

@monarchdodra
Copy link
Contributor

.__postblit?

That's only the "top level" call: It's not recursive. I think there might have also been an issue of using it with static arrays? Not sure. Denis-sh ahd submitted a pull for said functionality: dlang/phobos#928

Reading it again, it actually looks not that complicated to incorporate into druntime, I think.

EDIT: http://dpaste.dzfl.pl/f38b3c264cd2

@monarchdodra
Copy link
Contributor

I threw this together.

auto dup(T)(T[] a)
{
    alias UT = Unqual!T;
    enum message = "Cannot dup " ~ T.stringof ~ " to " ~ UT.stringof;
    static assert(is(T : UT), message ~ ".");
    static assert(is(typeof({UT ut = a[0];})), message ~ ": No acceptable posblit");

    if (__ctfe)
    {
        UT[] b;
        foreach(e; a)
            b ~= e;
        return b;
    }

    static if (is(typeof({UT ut;})))
    {
        auto b = new UT[a.length];
        b[] = a[];
        return b;
    }
    else
        return dupImpl!UT(a);
}

immutable(T)[] idup(T)(T[] a)
{
    alias UT = Unqual!T;
    alias IT = immutable(T);
    enum message = "Cannot idup " ~ T.stringof ~ " to " ~ IT.stringof;
    static assert(is(T : IT), message ~ ".");
    static assert(is(typeof({IT it = a[0];})), message ~ ": No acceptable posblit");

    if (__ctfe)
    {
        IT[] b;
        foreach(e; a)
            b ~= e;
        return b;
    }

    static if (is(typeof({UT ut;})))
    {
        static auto trustedCast(T, S)(S[] s)@trusted{return cast(T[])s;}
        auto b = new UT[a.length];
        b[] = trustedCast!UT(a)[];
        return trustedCast!IT(b);
    }
    else
        return dupImpl!IT(a);
}

private T[] dupImpl(T, S)(S[] a) @trusted
{
    void[] b = _adDupT(typeid(T[]), *cast(void[]*)&a);
    return *cast(T[]*)&b;
}

It's not pretty, and a bit longer, but it seems to cover all cases:

  • types with indirections
  • types without default init
  • ctfe support.

It requires Unqual, but that's just a 10 line template with no dependencies.

This is the tests I ran:

struct S
{
    int* p;
}

struct S2
{
    @disable this();
}

struct S3
{
    @disable this(this);
}

void main()
{
    int dg1() pure nothrow @safe //Correctlyinfered
    {
        {
           char[] m;
           string i;
           dup(m);
           idup(i);
           dup(i);
           idup(m);
        }
        {
           S[]            m;
           immutable(S)[] i;
           dup(m);
           idup(i);
           static assert(!is(typeof(idup(m))));
           static assert(!is(typeof(dup(i))));
        }
        {
           S3[]            m;
           immutable(S3)[] i;
           static assert(!is(typeof(dup(m))));
           static assert(!is(typeof(idup(i))));
        }
        return 1;
    }
    int dg2() pure /+nothrow+/ @safe //Fallback to typeid
    {
        {
           S2[]            m = [S2.init, S2.init];
           immutable(S2)[] i = [S2.init, S2.init];
           dup(m);
           dup(i);
           idup(m);
           idup(i);
        }
        return 1;
    }
    dg1();
    dg2();
    enum a = dg1();
    enum b = dg2();
}

Would this be an acceptable "start point"? Anything less I can absolutely guarantee would break phobos.

@MartinNowak
Copy link
Member

I've come up with a much more complete implementation, so I'll close this.
#760

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants