June 20, 2011
I have submitted a fix for bug 5272, http://d.puremagic.com/issues/show_bug.cgi?id=5272 "Postblit not called on copying due to array append"

However, I am starting to realize that one of the major reasons for postblit is to match it with an equivalent dtor.

This works well when the struct is on the stack -- the posblit for instance increments a reference counter, then the dtor decrements the ref counter.

But when the data is on the heap, the destructor is *not* called.  So what happens to any ref-counted data that is on the heap?  It's never decremented.  Currently though, it might still work, because postblit isn't called when the data is on the heap!  So no increment, no decrement.

I think this is an artificial "success".  However, if the pull request I initiated is accepted, then postblit *will* be called on heap allocation, for instance if you append data.  This will further highlight the fact that the destructor is not being called.

So is it worth adding calls to postblit, knowing that the complement destructor is not going to be called?  I can see in some cases where it would be expected, and I can see other cases where it will be difficult to deal with.  IMO, the difficult cases are already broken anyways, but it just seems like they are not.

The other part of this puzzle that is missing is array assignment, for example a[] = b[] does not call postblits.  I cannot fix this because _d_arraycopy does not give me the typeinfo.

Anyone else have any thoughts?  I'm mixed as to whether this patch should be accepted without more comprehensive GC/compiler reform.  I feel its a step in the right direction, but that it will upset the balance in a few places (particularly ref-counting).

-Steve
June 20, 2011
Steven Schveighoffer:

> The other part of this puzzle that is missing is array assignment, for example a[] = b[] does not call postblits.  I cannot fix this because _d_arraycopy does not give me the typeinfo.

This seems fixable. Is it possible to rewrite _d_arraycopy?


> Anyone else have any thoughts?

I think the current situation is not acceptable. This is a problem quite worse than _d_arraycopy because here some information is missing. Isn't this is the same problem with struct destructors?

A solution is to add this information at runtime, a type tag to structs that have a postblit and/or destructor. But then structs aren't PODs any more. There are other places to store this information, like in some kind of associative array.

Another solution is to forbid what the compiler can't guarantee. If a struct is going to be used only where its type is known, then it's allowed to have postblit and destructor. Is it possible to enforce this? I think it is. Here an @annotation is useful to better manage this contract between programmer and compiler.

Bye,
bearophile
June 20, 2011
On Mon, 20 Jun 2011 11:03:27 -0400, bearophile <bearophileHUGS@lycos.com> wrote:

> Steven Schveighoffer:
>
>> The other part of this puzzle that is missing is array assignment, for
>> example a[] = b[] does not call postblits.  I cannot fix this because
>> _d_arraycopy does not give me the typeinfo.
>
> This seems fixable. Is it possible to rewrite _d_arraycopy?

The compiler is the one passing the parameters to _d_arraycopy, so even if I change _d_arraycopy to accept a TypeInfo, the compiler needs to be fixed to send the TypeInfo.

I think this is really a no-brainer, because currently what is passed is the element size, which is contained within the TypeInfo.  I will be filing a bug on that.  But currently, I can't fix it.

>
>
>> Anyone else have any thoughts?
>
> I think the current situation is not acceptable. This is a problem quite worse than _d_arraycopy because here some information is missing. Isn't this is the same problem with struct destructors?

This is an easy fix -- the typeinfo contains information of whether or not and how to run the postblit.  The larger problem is the GC not calling the destructor.

But my immediate question is -- is it better to half-fix the problem by committing my changes, or leave the issue alone?

> A solution is to add this information at runtime, a type tag to structs that have a postblit and/or destructor. But then structs aren't PODs any more. There are other places to store this information, like in some kind of associative array.

Any solution that fixes the GC problem will have to store the typeinfo somehow associated with the block.  I think we may have more traction for this problem with a precise GC.

I don't think the right route is to store type info inside the struct itself.  This added overhead is not necessary for when the struct is stored on the stack.

> Another solution is to forbid what the compiler can't guarantee. If a struct is going to be used only where its type is known, then it's allowed to have postblit and destructor. Is it possible to enforce this? I think it is. Here an @annotation is useful to better manage this contract between programmer and compiler.

This is a possibility, making a struct only usable if it's inside another such struct or inside a class, or on the stack.

-Steve
June 20, 2011
On 2011-06-20 11:56, Jose Armando Garcia wrote:
> On Mon, Jun 20, 2011 at 12:03 PM, bearophile <bearophileHUGS@lycos.com>
wrote:
> > Steven Schveighoffer:
> > A solution is to add this information at runtime, a type tag to structs
> > that have a postblit and/or destructor. But then structs aren't PODs any
> > more. There are other places to store this information, like in some
> > kind of associative array.
> 
> What are PODs?

Plain Old Datatype. It's a user-defined data type with member variables but no functions. It just holds data.

- Jonathan M Davis
June 20, 2011
On 2011-06-20 10:34:14 -0400, "Steven Schveighoffer" <schveiguy@yahoo.com> said:

> I have submitted a fix for bug 5272,  http://d.puremagic.com/issues/show_bug.cgi?id=5272 "Postblit not called on  copying due to array append"
> 
> However, I am starting to realize that one of the major reasons for  postblit is to match it with an equivalent dtor.
> 
> This works well when the struct is on the stack -- the posblit for  instance increments a reference counter, then the dtor decrements the ref  counter.
> 
> But when the data is on the heap, the destructor is *not* called.  So what  happens to any ref-counted data that is on the heap?  It's never  decremented.  Currently though, it might still work, because postblit  isn't called when the data is on the heap!  So no increment, no decrement.
> 
> I think this is an artificial "success".  However, if the pull request I  initiated is accepted, then postblit *will* be called on heap allocation,  for instance if you append data.  This will further highlight the fact  that the destructor is not being called.
> 
> So is it worth adding calls to postblit, knowing that the complement  destructor is not going to be called?  I can see in some cases where it 
>  would be expected, and I can see other cases where it will be difficult to  deal with.  IMO, the difficult cases are already broken anyways, but it  just seems like they are not.
> 
> The other part of this puzzle that is missing is array assignment, for  example a[] = b[] does not call postblits.  I cannot fix this because  _d_arraycopy does not give me the typeinfo.
> 
> Anyone else have any thoughts?  I'm mixed as to whether this patch should  be accepted without more comprehensive GC/compiler reform.  I feel its a  step in the right direction, but that it will upset the balance in a few  places (particularly ref-counting).

My feeling is that array appending and array assignment should be considered a compiler issue first and foremost. The compiler needs to be fixed, and once that's done the runtime will need to be updated anyway to match the changes in the compiler. Your proposed fix for array assignment is a good start for when the compiler will provide the necessary info to the runtime, but applying it at this time will just fix some cases by breaking a few others: net improvement zero.

As for the issue that destructors aren't called for arrays on the heap, it's a serious problem. But it's also a separate problem that concerns purely the runtime, as far as I am aware of. Is there someone working on it?

-- 
Michel Fortin
michel.fortin@michelf.com
http://michelf.com/

June 20, 2011
On Mon, 20 Jun 2011 16:45:44 -0400, Michel Fortin <michel.fortin@michelf.com> wrote:

> On 2011-06-20 10:34:14 -0400, "Steven Schveighoffer" <schveiguy@yahoo.com> said:
>
>> I have submitted a fix for bug 5272,  http://d.puremagic.com/issues/show_bug.cgi?id=5272 "Postblit not called on  copying due to array append"
>>  However, I am starting to realize that one of the major reasons for  postblit is to match it with an equivalent dtor.
>>  This works well when the struct is on the stack -- the posblit for  instance increments a reference counter, then the dtor decrements the ref  counter.
>>  But when the data is on the heap, the destructor is *not* called.  So what  happens to any ref-counted data that is on the heap?  It's never  decremented.  Currently though, it might still work, because postblit  isn't called when the data is on the heap!  So no increment, no decrement.
>>  I think this is an artificial "success".  However, if the pull request I  initiated is accepted, then postblit *will* be called on heap allocation,  for instance if you append data.  This will further highlight the fact  that the destructor is not being called.
>>  So is it worth adding calls to postblit, knowing that the complement  destructor is not going to be called?  I can see in some cases where it  
>>  would be expected, and I can see other cases where it will be difficult to  deal with.  IMO, the difficult cases are already broken anyways, but it  just seems like they are not.
>>  The other part of this puzzle that is missing is array assignment, for  example a[] = b[] does not call postblits.  I cannot fix this because  _d_arraycopy does not give me the typeinfo.
>>  Anyone else have any thoughts?  I'm mixed as to whether this patch should  be accepted without more comprehensive GC/compiler reform.  I feel its a  step in the right direction, but that it will upset the balance in a few  places (particularly ref-counting).
>
> My feeling is that array appending and array assignment should be considered a compiler issue first and foremost. The compiler needs to be fixed, and once that's done the runtime will need to be updated anyway to match the changes in the compiler. Your proposed fix for array assignment is a good start for when the compiler will provide the necessary info to the runtime, but applying it at this time will just fix some cases by breaking a few others: net improvement zero.

BTW, I now feel that your request to make a distinction between move and copy is not required.  The compiler currently calls the destructor of temporaries, so it should also call postblit.  I don't think it can make the distinction between array appending and simply calling some other function.

If the issue of array assignment is fixed, do you think it's worth putting the change in, and then filing a bug against the GC?  I still think the current cases that "work" are fundamentally broken anyways.

For instance, in a ref-counted struct, if you appended it to an array, then removed all the stack-based references, the ref count goes to zero, even though the array still has a reference (I think someone filed a bug against std.stdio.File for this).

> As for the issue that destructors aren't called for arrays on the heap, it's a serious problem. But it's also a separate problem that concerns purely the runtime, as far as I am aware of. Is there someone working on it?

I think we need precise scanning to get a complete solution.  Another option is to increase the information the array runtime stores in the memory block (currently it only stores the "used" length) and then hook the GC to call the dtors.  This might be a quick fix that doesn't require precise scanning, but it also fixes the most common case of allocating a single struct or an array of structs on the heap.

-Steve
June 20, 2011
On 2011-06-20 15:12, Steven Schveighoffer wrote:
> On Mon, 20 Jun 2011 16:45:44 -0400, Michel Fortin
> 
> <michel.fortin@michelf.com> wrote:
> > On 2011-06-20 10:34:14 -0400, "Steven Schveighoffer"
> > 
> > <schveiguy@yahoo.com> said:
> >> I have submitted a fix for bug 5272, http://d.puremagic.com/issues/show_bug.cgi?id=5272 "Postblit not called on copying due to array append"
> >> 
> >> However, I am starting to realize that one of the major reasons for
> >> 
> >> postblit is to match it with an equivalent dtor.
> >> 
> >> This works well when the struct is on the stack -- the posblit for
> >> 
> >> instance increments a reference counter, then the dtor decrements the ref counter.
> >> 
> >> But when the data is on the heap, the destructor is *not* called. So
> >> 
> >> what happens to any ref-counted data that is on the heap? It's never decremented. Currently though, it might still work, because postblit isn't called when the data is on the heap! So no increment, no decrement.
> >> 
> >> I think this is an artificial "success". However, if the pull request
> >> 
> >> I initiated is accepted, then postblit *will* be called on heap allocation, for instance if you append data. This will further highlight the fact that the destructor is not being called.
> >> 
> >> So is it worth adding calls to postblit, knowing that the complement
> >> 
> >> destructor is not going to be called? I can see in some cases where it
> >> 
> >> would be expected, and I can see other cases where it will be
> >> 
> >> difficult to deal with. IMO, the difficult cases are already broken anyways, but it just seems like they are not.
> >> 
> >> The other part of this puzzle that is missing is array assignment,
> >> 
> >> for example a[] = b[] does not call postblits. I cannot fix this because _d_arraycopy does not give me the typeinfo.
> >> 
> >> Anyone else have any thoughts? I'm mixed as to whether this patch
> >> 
> >> should be accepted without more comprehensive GC/compiler reform. I feel its a step in the right direction, but that it will upset the balance in a few places (particularly ref-counting).
> > 
> > My feeling is that array appending and array assignment should be considered a compiler issue first and foremost. The compiler needs to be fixed, and once that's done the runtime will need to be updated anyway to match the changes in the compiler. Your proposed fix for array assignment is a good start for when the compiler will provide the necessary info to the runtime, but applying it at this time will just fix some cases by breaking a few others: net improvement zero.
> 
> BTW, I now feel that your request to make a distinction between move and copy is not required. The compiler currently calls the destructor of temporaries, so it should also call postblit. I don't think it can make the distinction between array appending and simply calling some other function.

If an object is moved, neither the postblit nor the destructor should be called. The object is moved, not copied and destroyed. I believe that TDPL is very specific on that.

- Jonathan M Davis
June 20, 2011
On Mon, 20 Jun 2011 18:43:30 -0400, Jonathan M Davis <jmdavisProg@gmx.com>
wrote:

> On 2011-06-20 15:12, Steven Schveighoffer wrote:
>> BTW, I now feel that your request to make a distinction between move and
>> copy is not required. The compiler currently calls the destructor of
>> temporaries, so it should also call postblit. I don't think it can make
>> the distinction between array appending and simply calling some other
>> function.
>
> If an object is moved, neither the postblit nor the destructor should be
> called. The object is moved, not copied and destroyed. I believe that TDPL is
> very specific on that.

Well, I think in this case it is being copied.  It's put on the stack, and
then copied to the heap inside the runtime function.  The runtime could be
passed a flag indicating the append is really a move, but I'm not sure
it's a good choice.  To me, not calling the postblit and dtor on a moved
struct is an optimization, no?  And you can't re-implement these semantics
for a normal function.  The one case I can think of is when an rvalue is
allowed to be passed by reference (which is exactly what's happening here).

Is there anything a postblit is allowed to do that would break a struct if
you disabled the postblit in this case?  I'm pretty sure internal pointers
are not supported, especially if move semantics do not call the postblit.

-Steve
June 21, 2011
On 2011-06-20 18:12:11 -0400, "Steven Schveighoffer" <schveiguy@yahoo.com> said:

> On Mon, 20 Jun 2011 16:45:44 -0400, Michel Fortin  <michel.fortin@michelf.com> wrote:
> 
>> My feeling is that array appending and array assignment should be  considered a compiler issue first and foremost. The compiler needs to be  fixed, and once that's done the runtime will need to be updated anyway  to match the changes in the compiler. Your proposed fix for array  assignment is a good start for when the compiler will provide the  necessary info to the runtime, but applying it at this time will just  fix some cases by breaking a few others: net improvement zero.
> 
> BTW, I now feel that your request to make a distinction between move and  copy is not required.  The compiler currently calls the destructor of  temporaries, so it should also call postblit.  I don't think it can make  the distinction between array appending and simply calling some other  function.

Well, if

	a ~= S();

does result in a temporary which get copied and then destroyed, why have move semantics at all? Move semantics are not just an optimization, they actually change the semantics. If you have a struct with a @disabled postblit, should it still be appendable?


> If the issue of array assignment is fixed, do you think it's worth putting  the change in, and then filing a bug against the GC?  I still think the  current cases that "work" are fundamentally broken anyways.

That depends. I'm not too sure currently whether the S destructor is called for this code:

	a ~= S();

If the compiler currently calls the destructor on the temporary S struct, then your patch is actually a fix because it balances constructors and destructors correctly for the appending part (the bug is then that compiler should use move semantics but is using copy instead). If it doesn't call the destructor then your patch does introduce a bug for this case.

All in all, I don't think it's important enough to justify we waste hours debating in what order we should fix those bugs. Do what you think is right. If it becomes a problem or it introduces a bug here or there, we'll adjust, at worse that means a revert of your commit.


>> As for the issue that destructors aren't called for arrays on the heap, 
>>  it's a serious problem. But it's also a separate problem that concerns 
>>  purely the runtime, as far as I am aware of. Is there someone working on  it?
> 
> I think we need precise scanning to get a complete solution.  Another  option is to increase the information the array runtime stores in the  memory block (currently it only stores the "used" length) and then hook 
>  the GC to call the dtors.  This might be a quick fix that doesn't require  precise scanning, but it also fixes the most common case of allocating a  single struct or an array of structs on the heap.

The GC calling the destructor doesn't require precise scanning. Although it's true that both problems require adding type information to memory blocks, beyond that requirement they're both independent. It'd be really nice if struct destructors were called correctly.


-- 
Michel Fortin
michel.fortin@michelf.com
http://michelf.com/

June 21, 2011
On 2011-06-20 16:07, Steven Schveighoffer wrote:
> On Mon, 20 Jun 2011 18:43:30 -0400, Jonathan M Davis <jmdavisProg@gmx.com>
> 
> wrote:
> > On 2011-06-20 15:12, Steven Schveighoffer wrote:
> >> BTW, I now feel that your request to make a distinction between move and copy is not required. The compiler currently calls the destructor of temporaries, so it should also call postblit. I don't think it can make the distinction between array appending and simply calling some other function.
> > 
> > If an object is moved, neither the postblit nor the destructor should be
> > called. The object is moved, not copied and destroyed. I believe that
> > TDPL is
> > very specific on that.
> 
> Well, I think in this case it is being copied.  It's put on the stack, and then copied to the heap inside the runtime function.  The runtime could be passed a flag indicating the append is really a move, but I'm not sure it's a good choice.  To me, not calling the postblit and dtor on a moved struct is an optimization, no?  And you can't re-implement these semantics for a normal function.  The one case I can think of is when an rvalue is allowed to be passed by reference (which is exactly what's happening here).

Well, going from the stack to the heap probably is a copy. But moves shouldn't be calling the postblit or the destructor, and you seemed to be saying that they should. The main place that a move would occur that I can think would be when returning a value from a function, which is very different. And I don't think that avoiding the postblit is necessarily just an optimization. If the postblit really is skipped, then it's probably possible to return an object which cannot legally be copied (presumably due to some combination of reference or pointer member variables and const or immutable), though that wouldn't exactly be a typical situation, even if it actually is possible. It _is_ primarily an optimization to move rather than copy and destroy, but I'm not sure that it's _just_ an optimization.

> Is there anything a postblit is allowed to do that would break a struct if you disabled the postblit in this case?  I'm pretty sure internal pointers are not supported, especially if move semantics do not call the postblit.

If the struct had a pointer to a local member variable which the postblit would have deep-copied, then sure, not calling the postblit would screw with the struct. But that would screw with a struct which was returned from a function as well, and that's the prime place for the move semantics. That sort of struct is just plain badly designed, so I don't think that it's really something to worry about. I can't think of any other cases where it would be a problem though. Structs don't usually care where they live (aside from the issue of structs being designed to live on the stack and then not getting their destructor called because they're on the heap).

- Jonathan M Davis
« First   ‹ Prev
1 2 3
Top | Discussion index | About this forum | D home