If I use GC.free on a large array of data, it is not freed.
example:
auto x = new int[10000];
GC.free(x.ptr);
assert(GC.addrOf(x.ptr) == null); // fails
The last line works in 2.066.1
If I use delete on that array, and it's an array of structs, it will call the dtor twice on that array.
Note that x.ptr is an interior pointer because of the array padding. I'm not sure if the old GC took this into account or what, but it seems to work.
Comment #1 by r.sagitario — 2015-02-06T16:56:40Z
That was a bug in 2.066.
Contrary to the documentation free() didn't really check whether a pointer is to the base of the allocation, and this could seriously corrupt the internal data structures.
The check now is better, but still not perfect. The GC cannot easily determine whether a small allocation is already free'd or not.
> If I use delete on that array, and it's an array of structs, it will call the dtor twice on that array.
The new struct destruction by the GC assumes that, if you manually call the destructor, you also take care of manually freeing the memory with GC.free. That way the GC will not call the destructor again.
Comment #2 by r.sagitario — 2015-02-06T17:12:10Z
I admit there is a problem in _d_delarray_t which is called for "delete array;". Should it free the whole block instead using GC.query.base? What if the array is just a slice of a larger array?
Comment #3 by schveiguy — 2015-02-06T18:33:49Z
OK, so I have a problem with the current code:
1. When we say new T[x], we get a pointer not to the point of the block
2. delete is supposedly going to be deprecated.
So if the code stays as it is, our promoted method of destroying an array is no good. There is no good way to destroy an array except delete.
I think we definitely should fix _d_delarray_t, as dtors should not be called more than once, and to have it not free the data is bad.
But I think we need some way of saying this block can still be freed because even though it's not the base pointer, the data before the pointer is owned by the runtime, and invisible to the user. And I really think that should be done in GC.free.
Comment #4 by schveiguy — 2015-02-06T18:42:24Z
(In reply to Steven Schveighoffer from comment #3)
> 1. When we say new T[x], we get a pointer not to the point of the block
... not to the *front* of the block
Comment #5 by r.sagitario — 2015-02-06T19:04:31Z
Maybe we should relax the restriction on base pointers for free().
What should happen with code like this?
struct S { ~this() { printf("~this\n"); } int x; }
S[] arr = new S[1000];
S[] slice = arr[100..200];
S[] tail = arr[200..1000];
delete slice;
Should it call call only 100 destructors? All 1000? Is this invalid code?
Calling dtors twice could be fixed by explicitely removing the FINALIZE attribute from the block, but could not work for just a part of the array.
Comment #6 by schveiguy — 2015-02-06T19:21:10Z
(In reply to Rainer Schuetze from comment #5)
> Maybe we should relax the restriction on base pointers for free().
>
> What should happen with code like this?
>
> struct S { ~this() { printf("~this\n"); } int x; }
>
> S[] arr = new S[1000];
> S[] slice = arr[100..200];
> S[] tail = arr[200..1000];
> delete slice;
>
> Should it call call only 100 destructors? All 1000? Is this invalid code?
Good question. I would tend to think that there are several right answers:
1. Destroy entire array.
2. Destroy only the slice, do not run dtors on the head or tail
3. Error, must delete entire array
4. Do nothing (and don't free the array).
Wrong thing to do would be run destructor on slice and run it again when the memory is freed.
My preference is for option 3, but that may be too heavy handed.
Comment #7 by r.sagitario — 2015-02-06T20:00:46Z
Checking the code from 2.066 again, it corrupts memory
- if the array.ptr offset from the base pointer in a small allocation is larger than 15 bytes
- if the array.ptr offset from the base pointer in a large allocation is larger than 4095 bytes.
So apart from some rather obscure indices above 0, but below some threshold, only passing the array base pointer actually worked. Making other pointers raise an error seems an improvement.
For a partial length, it didn't make much of a difference unless struct destructors have to be called. Not calling all of them is very probably an error, too. Informing the user about this with an error should be ok.
This leaves the case when a wrong size didn't cause any issues. An error could be considered annoying, so maybe we could relax option 3 a little for types without destructor.
Comment #8 by r.sagitario — 2015-02-06T20:04:55Z
Other notes:
- The current version also "works" for memory allocated outside the GC, it just doesn't free the memory.
- The current version "works" for manually allocated arrays (e.g. Appender).
>So if the code stays as it is, our promoted method of destroying an array is no good. There is no good way to destroy an array except delete.
What is the "proposed method"? Using GC.free(arr.ptr)?
Comment #9 by code — 2015-02-11T18:10:31Z
(In reply to Rainer Schuetze from comment #8)
> Other notes:
>
> - The current version also "works" for memory allocated outside the GC, it
> just doesn't free the memory.
That should be fixed, we don't want to promote delete arr[] as a method to destroy a slice.
> There is no good way to destroy an array except delete.
foreach (ref el; ary)
destroy(el);
Comment #10 by code — 2015-02-11T18:16:19Z
I think the right semantic is to destroy+free the complete array iff it's GC managed.
Comment #11 by schveiguy — 2015-02-12T13:24:57Z
(In reply to Martin Nowak from comment #9)
> (In reply to Rainer Schuetze from comment #8)
> > Other notes:
> >
> > - The current version also "works" for memory allocated outside the GC, it
> > just doesn't free the memory.
>
> That should be fixed, we don't want to promote delete arr[] as a method to
> destroy a slice.
I agree.
>
> > There is no good way to destroy an array except delete.
>
> foreach (ref el; ary)
> destroy(el);
Some type of destroyRange would be a good addition (not just for arrays).
Regardless of this, we need to support a way to manually manage array memory. Especially large arrays, that is where the problem of the conservative GC really shows up.
When writing pull requests that fix a bugzilla issue, write the title as:
Fix Issue NNNN ....
so that the bugzilla issue ill be automatically marked as fixed when it gets pulled.
Comment #15 by github-bugzilla — 2015-02-18T03:39:13Z