std.typecons.scoped problems

[dlangBugzillaToGithub]

bearophile_hugs reported this on 2010-10-24T19:10:07Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=5115

CC List

• andrej.mitrovich (@AndrejMitrovic)
• braddr
• dmitry.olsh (@DmitryOlshansky)
• post (@kyllingstad)
• yebblies

Description

The D type system doesn't know about the scoped nature of objects built by std.typecons.scoped, this gives two main disadvantages over the (deprecated) "scope" classes:
- The class destructor isn't called
- The compiler doesn't and can't enforce that references to the class don't escape the scope.


The first problem may be seen with this test program, changing the values of the three static if:


import std.typecons: scoped;
import std.conv: emplace;
import std.c.stdio: puts;
import std.c.stdlib: exit;

class Foo {
    this() { puts("constructor"); }
    ~this() { puts("destructor"); }
}

void main() {
    {
        static if (1) {
            scope Foo f = new Foo(); // shows "destructor"
        }

        static if (0) {
            Foo f = scoped!Foo(); // doesn't show "destructor"
        }

        static if (0) {
            // doesn't show "destructor"
            ubyte[__traits(classInstanceSize, Foo)] buf = void;
            Foo f = emplace!(Foo)(cast(void[])buf);
        }
    }

    //exit(0);
}


A temporary patch to this problem is to add a template constraint to std.typecons.scoped, that refuses to instantiate the template if the given class has a destructor. This means replacing:

@system auto scoped(T, Args...)(Args args) if (is(T == class))
{


With:

@system auto scoped(T, Args...)(Args args) if (is(T == class) &&
                                               !__traits(hasMember, T, "__dtor"))
{

---------------------------

The second problem may be seen with this program:


import std.typecons: scoped;
import std.conv: emplace;

class Foo {}

Foo bar() {
    static if (1) {
        // shows: Error: escaping reference to scope local f
        scope Foo f = new Foo();
    }

    static if (0) {
        // shows no errors
        Foo f = scoped!Foo();
    }

    static if (0) {
        // shows no errors
        ubyte[__traits(classInstanceSize, Foo)] buf = void;
        Foo f = emplace!(Foo)(cast(void[])buf);
    }

    return f;
}

void main() {}


In the first case the DMD 2.049 compiler catches the bug, while in the two other cases no errors are generated.

-------------------------------

I guess that once std.typecons.scoped works well, the std.conv.emplace may be removed from Phobos.

-------------------------------

The main purpose of "scoped" is to increase performance, allowing stack allocation of a class instance (the Java VM solves this problem becoming smarter, performing escape analysis and stack-allocating object instances that don't escape the current scope. But this is not exactly the same thing as a well implemented scope/scoped feature, because the scoped documents that the programmer wants a class to be stack-allocated, this means that the compiler has to generate an error if a reference to this class escapes. While in Java if the programmer wants a scoped class instance, then the programmer has to be careful that no references escape, otherwise the class instance will be silently heap allocated).

So this is benchmark code that may be used to compare the performance of the various implementations. To give a meaningful reference, the timings of this D benchmark must be compared to the baseline timings of the Java code below, run on a updated JavaVM using the -release switch too.



// D2 code
import std.typecons: scoped;
import std.conv: emplace;

final class Test { // 32 bytes each instance
    int i1, i2, i3, i4, i5, i6;
    this(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
        this.i1 = ii1;
        this.i2 = ii2;
        this.i3 = ii3;
        this.i4 = ii4;
        this.i5 = ii5;
        this.i6 = ii6;
    }
    void doSomething(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
    }
}

void main() {
    enum int N = 10_000_000;

    int i;
    while (i < N) {
        static if (1) { // 2.70 seconds, without testObject=null; 2.71 seconds
            Test testObject = new Test(i, i, i, i, i, i);
        }

        static if (0) { // 0.66 seconds, without testObject=null; 1.31 seconds
            scope Test testObject = new Test(i, i, i, i, i, i);
        }

        static if (0) { // 0.42 seconds, without testObject=null; 0.42 seconds
                        // 0.24 seconds if I comment out the enforces inside emplace!()
            ubyte[__traits(classInstanceSize, Test)] buf = void;
            Test testObject = emplace!(Test)(cast(void[])buf, i, i, i, i, i, i);
        }

        static if (0) { // 0.69 // without testObject=null; 0.69 seconds
            Test testObject = scoped!Test(i, i, i, i, i, i);
        }

        static if (0) { // (Test is a struct) 0.19 seconds
            Test testObject = Test(i, i, i, i, i, i);
        }

        testObject.doSomething(i, i, i, i, i, i);
        testObject.doSomething(i, i, i, i, i, i);
        testObject.doSomething(i, i, i, i, i, i);
        testObject.doSomething(i, i, i, i, i, i);
        testObject = null;
        i++;
    }
}


--------------------------

// Java code
final class Obj {
    int i1, i2, i3, i4, i5, i6;

    Obj(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
        this.i1 = ii1;
        this.i2 = ii2;
        this.i3 = ii3;
        this.i4 = ii4;
        this.i5 = ii5;
        this.i6 = ii6;
    }

    void doSomething(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
    }
}

final class Test {
    public static void main(String args[]) {
        final int N = 10_000_000;
        int i = 0;
        while (i < N) {
            Obj testObject = new Obj(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            // testObject = null; // no difference
            i++;
        }
    }
}


--------------------------

Currently the version that uses emplace is the fastest in D if all enforce() are commented out from the code of emplace itself.

Being emplace a template (so it is not statically compiled into the Phobos lib, but compiled every time, this means that asserts are keeps inside it or removed according to the user compilation switches), and being its purpose essentially for performance, I suggest to replace all enforce() inside emplace() with normal asserts.


A possible solution to the problems of scoped is to add back some compiler support to this feature, so the compiler is more aware of the situation. (For example a @scoped type attribute, that just forbid a class reference to escape the current scope. So all objects instantiated by std.typecons.scoped automatically have this attribute.)

[dlangBugzillaToGithub]

bearophile_hugs commented on 2010-12-19T13:06:00Z

An interesting example found by Andrej Mitrovic (modified):


import std.c.stdio: puts;
import std.typecons: scoped;
class Foo {
    ~this() {
        puts("Foo.dtor");
    }
}
void main() {
    Foo f1 = scoped!Foo();  // doesn't call dtor after the scope
    auto f2 = scoped!Foo(); // calls dtor after the scope
}

[dlangBugzillaToGithub]

yebblies commented on 2010-12-20T21:00:29Z

(In reply to comment #1)
> An interesting example found by Andrej Mitrovic (modified):
> 
> 
> import std.c.stdio: puts;
> import std.typecons: scoped;
> class Foo {
>     ~this() {
>         puts("Foo.dtor");
>     }
> }
> void main() {
>     Foo f1 = scoped!Foo();  // doesn't call dtor after the scope
>     auto f2 = scoped!Foo(); // calls dtor after the scope
> }

This example (and the first half of this bug report) appears to be a duplicate of bug http://d.puremagic.com/issues/show_bug.cgi?id=3516

[dlangBugzillaToGithub]

bearophile_hugs commented on 2010-12-21T00:22:23Z

(In reply to comment #2)

> > void main() {
> >     Foo f1 = scoped!Foo();  // doesn't call dtor after the scope
> >     auto f2 = scoped!Foo(); // calls dtor after the scope
> > }
> 
> This example (and the first half of this bug report) appears to be a duplicate
> of bug http://d.puremagic.com/issues/show_bug.cgi?id=3516

But both f1 and f2 gets assigned. I don't fully understand issue 3516 so I can't tell.

[dlangBugzillaToGithub]

braddr commented on 2010-12-21T00:29:38Z

Any issue that involves a struct dtor not being properly run is almost certainly an extension of bug 3516.  Walter's well aware of it, but it's a non-trivial problem to fix.

[dlangBugzillaToGithub]

post (@kyllingstad) commented on 2010-12-21T00:48:45Z

Isn't the problem rather that the type of f2 is Scoped!Foo, while the type of f1 is just Foo?  The compiler doesn't know that it should call the destructor of a Foo at the end of the scope.

[dlangBugzillaToGithub]

bearophile_hugs commented on 2010-12-21T00:52:48Z

(In reply to comment #5)
> Isn't the problem rather that the type of f2 is Scoped!Foo, while the type of
> f1 is just Foo?  The compiler doesn't know that it should call the destructor
> of a Foo at the end of the scope.

You are probably right. Then why isn't this a compile error?

Foo f1 = scoped!Foo();

[dlangBugzillaToGithub]

post (@kyllingstad) commented on 2010-12-21T00:56:37Z

(In reply to comment #6)
> You are probably right. Then why isn't this a compile error?
> 
> Foo f1 = scoped!Foo();

Because Scoped!Foo has an 'alias this' to the underlying Foo, which is necessary for it to behave like a Foo.

[dlangBugzillaToGithub]

yebblies commented on 2010-12-21T01:00:19Z

(In reply to comment #7)
> (In reply to comment #6)
> > You are probably right. Then why isn't this a compile error?
> > 
> > Foo f1 = scoped!Foo();
> 
> Because Scoped!Foo has an 'alias this' to the underlying Foo, which is
> necessary for it to behave like a Foo.

scoped!Foo() returns a temporary of type scoped!(Foo).Scoped (or something like that).
This temporary is implicitly converted to Foo using alias this, but the temporary never has it's destructor called due to bug 3516, which means Foo's destructor is never called either.

It is fine for the temporary to be converted to Foo, so long as the destructor is called when the scope that 'scoped' was called in is exited.

[dlangBugzillaToGithub]

k.hara.pg commented on 2011-11-10T23:11:38Z

(In reply to comment #8)
> scoped!Foo() returns a temporary of type scoped!(Foo).Scoped (or something like
> that).
> This temporary is implicitly converted to Foo using alias this, but the
> temporary never has it's destructor called due to bug 3516, which means Foo's
> destructor is never called either.
> 
> It is fine for the temporary to be converted to Foo, so long as the destructor
> is called when the scope that 'scoped' was called in is exited.

Now bug 3516 was fixed. Then comment#1 code prints two "Foo.dtor"

But allowing conversion from temporary type (e.g. Scoped!Foo) to Foo is unsafe, because the temporary has value type and its lifetime is limited in its scope, but Foo is class reference of the temporary and we can bring it out the scope.

Foo global_foo;
void test1() {
    auto foo = scoped!Foo();
    global_foo = foo;   // implicitly conversion from typeof(foo) to Foo
      // This line should be forbidden in compile time
}
void test2() {
    // use global_foo -> Access Violation!
}
void main() {
    test1();
    test2();
}

I think my ProxyOf mixin template is useful for this issue.
https://github.com/D-Programming-Language/phobos/pull/300

[dlangBugzillaToGithub]

k.hara.pg commented on 2011-11-10T23:11:39Z

(In reply to comment #8)
> scoped!Foo() returns a temporary of type scoped!(Foo).Scoped (or something like
> that).
> This temporary is implicitly converted to Foo using alias this, but the
> temporary never has it's destructor called due to bug 3516, which means Foo's
> destructor is never called either.
> 
> It is fine for the temporary to be converted to Foo, so long as the destructor
> is called when the scope that 'scoped' was called in is exited.

Now bug 3516 was fixed. Then comment#1 code prints two "Foo.dtor"

But allowing conversion from temporary type (e.g. Scoped!Foo) to Foo is unsafe, because the temporary has value type and its lifetime is limited in its scope, but Foo is class reference of the temporary and we can bring it out the scope.

Foo global_foo;
void test1() {
    auto foo = scoped!Foo();
    global_foo = foo;   // implicitly conversion from typeof(foo) to Foo
      // This line should be forbidden in compile time
}
void test2() {
    // use global_foo -> Access Violation!
}
void main() {
    test1();
    test2();
}

I think my ProxyOf mixin template is useful for this issue.
https://github.com/D-Programming-Language/phobos/pull/300

[dlangBugzillaToGithub]

andrej.mitrovich (@AndrejMitrovic) commented on 2012-12-18T13:02:40Z

(In reply to comment #10)
> Now bug 3516 was fixed. Then comment#1 code prints two "Foo.dtor"
> 
> But allowing conversion from temporary type (e.g. Scoped!Foo) to Foo is unsafe,
> because the temporary has value type and its lifetime is limited in its scope,
> but Foo is class reference of the temporary and we can bring it out the scope.

There is another problem. You may want to pass the instance to another function, which is ok since the type will still be alive during that call:

class C { }
void func(C c) { }
void main()
{
    auto c = scoped!C();
    func(c);  // ok, c is still alive here
}

Disabling implicit conversion (maybe by using your ProxyOf mixin) might be ok, but then we can no longer pass the instance around because `Scoped` is a voldemort type hidden inside the `scoped` function. E.g.:

class C { }
void func(C c) { }
void funcScoped(??? c) { }  // param needs to be a proper type
void main()
{
    auto c = scoped!C();  // voldemort type
    func(c);  // error, no implicit conversion
    funcScoped(c);  // would be ok if we knew what type c was
}

So if we disable implicit conversion we should probably introduce a Scoped type instead of a voldemort, so we can write:

void funcScoped(ref Scoped!C c) { }

[dlangBugzillaToGithub]

dmitry.olsh (@DmitryOlshansky) commented on 2012-12-19T09:15:42Z

(In reply to comment #11)
> 
> There is another problem. You may want to pass the instance to another
> function, which is ok since the type will still be alive during that call:
> 
> class C { }
> void func(C c) { }
> void main()
> {
>     auto c = scoped!C();
>     func(c);  // ok, c is still alive here
> }
> 
> Disabling implicit conversion (maybe by using your ProxyOf mixin) might be ok,
> but then we can no longer pass the instance around because `Scoped` is a
> voldemort type hidden inside the `scoped` function. E.g.:
> 
> class C { }
> void func(C c) { }
> void funcScoped(??? c) { }  // param needs to be a proper type
> void main()
> {
>     auto c = scoped!C();  // voldemort type
>     func(c);  // error, no implicit conversion
>     funcScoped(c);  // would be ok if we knew what type c was
> }
> 
> So if we disable implicit conversion we should probably introduce a Scoped type
> instead of a voldemort, so we can write:
> 
> void funcScoped(ref Scoped!C c) { }


Even more interesting is emplacing inside of class instance (to avoid pointer overhead while modeling composition):

class A{//A makes sure b & c are not escaped
   Scoped!B b;
   Scoped!C c;
}

Overall I think voldemort types are overpriced.

[dlangBugzillaToGithub]

dmitry.olsh (@DmitryOlshansky) commented on 2018-05-18T10:21:27Z

Indeed and with `scope` becoming a thing in DIP 1000 we can have our scope storage class to have stack-allocated classes.

scope a = new Blah(...);

scope class instances are everywhere in DMD frontend now as well.
It's plain as day that our library solution had failed and is not required anymore.