Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Improved semantics for destroy() #2126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

andralex
Copy link
Member

@andralex andralex commented Mar 2, 2018

This supersedes #2124 and previous attempts. Apologies for the override but it seemed easier to simply write this than discuss what to do over several sessions. cc @JinShil

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

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.

Detailed behavior of `destroy(obj)` depends on the type `T` as
follows:

$(UL $(LI If `T` is a primitive type, a `struct` types that does not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`struct` type (singular)

references any other objects. It does $(I not) initiate a GC cycle or free
any GC memory.
Destroys an object by means of calling its destructor, if present. For
types `T` that define a destructor, `destroy` then also fills the
Copy link
Contributor

@JinShil JinShil Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably best to use "... that have a destructor" here since that's the verbage used in the following "NOTE".

@JinShil
Copy link
Contributor

JinShil commented Mar 3, 2018

This is better than my prior understanding of @andralex's specification as, at least, reference types and value types are treated uniformly. However, I still don't understand why having a destructor or not having a destructor should result in different behavior.

And, it looks like we'll have to do something about Higgs. cc @sbstp See the Jenkins "details".

@sbstp
Copy link

sbstp commented Mar 4, 2018

I'm not the author of Higgs, you should probably cc @maximecb.

@maximecb
Copy link

maximecb commented Mar 4, 2018

Oha @JinShil.

So, it's been a long time since I've looked at this code, but this might be the source of your problem: https://github.com/higgsjs/Higgs/blob/master/source/runtime/gc.d#L914

@12345swordy
Copy link
Contributor

no attempts of fixing bugs regarding the class deconstructor when it comes to attributes?

@JinShil
Copy link
Contributor

JinShil commented Mar 7, 2018

Breaking Higgs

I removed one destructor from Higgs in an attempt to make it compatible with this PR, but there are still 3 more that need attention:

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/object.d#L512
https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/vm.d#L805
https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/gc.d#L87

Alternate Proposal

I propose decoupling destruction from re-initialization for all cases, and delegating the decision and method of re-initialization to the user. That puts the burden on the user to understand what they need to do and why, but I believe that's responsibility the user opts into when they choose to implement a destructor. Consider the following contrived examples.

Method 1: Use a boolean flag to indicate whether or not the object has already been destroyed

struct S 
{
    void* p;
    bool destroyed = false;
    this(size_t size)
    {
        p = allocateResource(size);  // may not have the same semantics as `malloc`
    }
    ~this() 
    {
        if (!destroyed)
        {
            deallocateResource(p);   // may not have the same semantics as `free`
            destroyed = true;
        }
    }
}

Method 2: Use a special state to indicate whether or not the object has already been destroyed

struct S 
{
    void* p;
    this(size_t size)
    {
        p = allocateResource(size);   // may not have the same semantics as `malloc`
    }
    ~this() 
    {
        if (p == null)
        {
            deallocateResource(p);   // may not have the same semantics as `free`
            p = null;
        }
    }
}

Method 3: Re-destuction is benign due to the semantics of the resource deallocation

struct S 
{
    void* p;
    this(size_t size)
    {
        p = allocateResource(size);  // same semantics as `malloc`
    }
    ~this() 
    {
        deallocateResource(p);     // same semantics as `free`
        p = null;
    }
}

Learning from C#

There may also be some wisdom to be found in C#'s dispose pattern documented here: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern. It is Microsoft's solution to basically the same problem.

@JinShil
Copy link
Contributor

JinShil commented Mar 7, 2018

no attempts of fixing bugs regarding the class deconstructor when it comes to attributes?

That's out of scope for now, and probably needs changes to be made in DMD.

@andralex
Copy link
Member Author

andralex commented Mar 8, 2018

@JinShil the destructors in Higgs look reasonable. In what way are they broken?

As a general disposition, we're going toward more, not less, safety. So the situation in which we make destroy leave the object in an unsafe state for destruction is not acceptable. Yes, per your examples there are numerous ways in which user code can make the destructor safe. (Those include of course overwriting the object with .init, which is exactly what destroy does!) But it is impossible for the compiler to assess that the changes make the destroy/~this combo safe.

@JinShil
Copy link
Contributor

JinShil commented Mar 8, 2018

In what way are they broken?

I believe it is their mere existence, causing the object to be re-initialized, that causes problems for the Higg's test suite. But, I need to look into it in more detail to be sure.

@JinShil
Copy link
Contributor

JinShil commented Mar 20, 2018

@andralex How confident are you in this implementation? For some reason, noone from team DRuntime seems very interested in it (or maybe you're just too scary). Are you willing to play the dictator with this, or are you looking for feedback?

@andralex
Copy link
Member Author

andralex commented Mar 20, 2018

@JinShil thanks for asking! :) I think the intent is what we need; of course it can be improved, and also the implementation may have issues. So feedback is welcome.

Generally destroy needs to rely on what we already have in the language instead of requiring new paraphernalia or asking the user to rewrite their destructors in a certain way.

The mechanisms at our disposal are the following:

  • Destructors get rid of an object's extra resources.
  • Destructors leave the object in an invalid state; adding a requirement here is not tenable.
  • Every object (even those with @disable this();) has a well-defined init state
  • The init state does not allocate any resources and can be copied with memcpy
  • At least for objects without @disable this();, an object in its initial state must be destroyable, otherwise trivial code such as { T t; } would be incorrect.
  • We can use introspection to figure out whether an object has a destructor or not.

The fundamental approach of this PR is simple - calling destroy should call the destructor, if any, and leave the object in a valid destroyable state. So if the object has no destructor there's no need to do anything at all - leave the object as is, it's as destroyable as it was before calling destroy. If the object does have a destructor we should call it and then repaint the object with its initial state - the one that we can reasonably assume it's safely destroyable. If we remove from this behavior we allow for unsafe code (people call destroy and then the destructor gets automatically invoked). If we add to this behavior we do work for naught (as was the case before - why should destroy on an int set it to zero? Every int has as much resources (none) and as many rights as others). If we add requirements to the destructor we break all existing destructors. If we add compiler support we incur overhead. There's really not a lot of wiggle room here.

I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves?

@wilzbach
Copy link
Member

I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves?

See Mike's comment above. They can be seen on the Project Tester:

https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fdruntime/detail/PR-2126/2/pipeline

For local reproduction, do sth. like:

git clone https://github.com/higgsjs/Higgs
cd Higgs
dub --compiler=/home/seb/dlang/dmd/generated/linux/release/64/dmd

@JinShil
Copy link
Contributor

JinShil commented Mar 21, 2018

I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves?

The symptom appears in the Higgs test suite where the unitttests are specifically testing if an object is in a certain state:

Assertion failed (runtime/gc.d@472): gcForward: object not in from-space heap

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/gc.d#L472

I believe this is caused simply by the existence of some of destructors in the Higgs code base. I already removed 1 empty constructor which solved one error, but another appeared. There are current 2 more empty constructors in Higgs:

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/object.d#L512
https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/vm.d#L805

I tried to remove those as well, but it didn't solve the problem we're currently experiencing.

The current error appears to be due to the following destructor, which is not empty:

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/gc.d#L87

I tried to fix that also to make it compatible with this PR, but ultimately failed. I don't understand the code or the test suite logic well enough to make the change.

If you are convinced that this PR is the way to go, and are willing to exercise your authority, given the lack of reviews, I propose the following procedure:

  1. Remove Higgs from the Jenkins test suite
  2. File an issue in the Higgs repository and leave it to the authors there to fix it
  3. Merge this PR
  4. If and when Higgs is fixed to make it compatible with this PR, we can re-add it to the Jenkins test suite.

Note that I am not a member of team DRuntime, so if you don't want to merge your own PR, you'll still have to convince someone to review this PR. I'm not crazy about these semantics, but I also don't have any other solution. I'd be hesitant putting my name next to the merge notice even if I were on team DRuntime, but I'd do it if enough time passed without anyone else stepping up with an alternate solution.

Pinging @klickverbot @schveiguy @MartinNowak @rainers @ZombineDev @DmitryOlshansky @maximecb for help.

@12345swordy
Copy link
Contributor

!ping

@ntrel
Copy link
Contributor

ntrel commented Jun 7, 2018

Optimization: If a class/struct aggregate does not define a destructor itself, but one of its fields defines a destructor, we need to do field = field.init so the field destructor can be called again later. The aggregate's generated destructor will not access any fields that don't have a destructor, so we do not need to reinit those. This pull seems to always reinit all fields of the aggregate (which is fine, but overkill).

Also, I like the new semantics of destroy, but I expect people have relied on it always doing the reinit. I read the discussion in #2115, and even if the doc description was vague (until recently?), the [documented] unittests were not - they clearly show reinit, implying that users can rely on it. I suggest this pull either uses a different function name or somehow uses deprecation to make users aware of the change.

@andralex
Copy link
Member Author

andralex commented Jun 7, 2018

@ntrel we won't cater to people relying on undocumented behavior. This not only creates difficulties for this particular PR, but creates a dangerous precedent whereby we dig ourselves in bureaucracy.

Copy link
Member

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve the general idea, but needs a few things, see comments.


$(UL $(LI If `T` is a primitive type, a `struct` types that does not
have a destructor, or a `class` type that does not have a destructor,
the call to has no effect.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"call to has no effect" Maybe missing "destroy" in there?

Note, this is technically incorrect for a class type with no destructor, as destroying a class instance that has no destructor is still going to call rt_finalize, which at least sets the vtable to null.

*/
void destroy(T)(T obj) if (is(T == class))
void destroy(T)(T obj) if (is(T == class) || (is(T == interface)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a pre-existing bug in here, as I think the static test will pass for extern(C++) classes, which is definitely NOT correct to call rt_finalize on. Something to think about as we move towards betterC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Objective-C classes. There’s a recent change where __traits(getLinkage) supports classes.

@@ -3145,9 +3231,8 @@ unittest

/// ditto
void destroy(T)(ref T obj)
if (!is(T == struct) && !is(T == interface) && !is(T == class) && !_isStaticArray!T)
if (!is(T == struct) && !is(T == interface) && !is(T == class) && !_isStaticArray!T)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to accept an lvalue dynamic array, which is potentially going to override the array code above. I'm not 100% sure, because it's a specialization.

This may fail:

static struct S
{
    int x;
    ~this() {}
}

S[] arr = new S[5];
arr[] = S(42);
destroy(arr);
assert(arr[0].x == 0); // will this fail?

@ntrel
Copy link
Contributor

ntrel commented Jun 9, 2018

people relying on undocumented behavior

@andralex I suggest we revert #2054 ASAP unless this is merged now. #2054 was merged on 27 Jan (it also says you approved those changes).

Observation: The hasElaborateDestroy changes and the destroy(T[]) changes are orthogonal and could have been separated.

@thewilsonator thewilsonator added the Needs Rebase needs a `git rebase` performed label May 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Rebase needs a `git rebase` performed Needs Work stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.