Destruction bug in make test.
Juergen Boemmels
I just discovered a really subtele bug:
Normaly the test are not run with --destroy-at-end. This has not many
consequences yet because the only PMCs with active destruction are
IOs, in fact only one test is really sensitive to t/pmc/io_4.pasm, it
won't flush its buffers without --destroy-at-end. The print op also
did not use this buffer so this bug didn't even show up.
There are several ways to solve this:
- run the test always with --destroy-at-end
- make --destroy-at-end the default and have an option --no-destroy-at-end
- add a explicit flush to the test just before the end.
I think we should take the second route: destroy-function should be
run by default at the end of program.
bye
boe
--
Juergen Boemmels boem...@physik.uni-kl.de
Fachbereich Physik Tel: ++49-(0)631-205-2817
Universitaet Kaiserslautern Fax: ++49-(0)631-205-3906
PGP Key fingerprint = 9F 56 54 3D 45 C1 32 6F 23 F6 C7 2F 85 93 DD 47
Dan Sugalski
> Hi,
>
> I just discovered a really subtele bug:
> Normaly the test are not run with --destroy-at-end. This has not many
> consequences yet because the only PMCs with active destruction are
> IOs, in fact only one test is really sensitive to t/pmc/io_4.pasm, it
> won't flush its buffers without --destroy-at-end. The print op also
> did not use this buffer so this bug didn't even show up.
>
> There are several ways to solve this:
> - run the test always with --destroy-at-end
> - make --destroy-at-end the default and have an option --no-destroy-at-end
> - add a explicit flush to the test just before the end.
>
> I think we should take the second route: destroy-function should be
> run by default at the end of program.
Option 2 is the right one. (Well, OK, having parrot do an explicit sweep &
destroy's the right option, but until then...) Go ahead and add a patch to
whatever you need to make this happen.
Dan
Juergen Boemmels
[...]
> Option 2 is the right one. (Well, OK, having parrot do an explicit sweep &
> destroy's the right option, but until then...) Go ahead and add a patch to
> whatever you need to make this happen.
It turned out, that it was even simpler. The explicit sweeping is
already in the code, but is only triggered if there are objects which
need _early_ destruction, not if objects just need
destruction. Removing this test made it work.
Fix is commited.
cu
Leopold Toetsch
I disagree :) We already have a 2 stage IO destroy. The first shall
flush its files. This get called even if destroy-at-end isn't set. The
second phase (clean allocated memory) isn't called for the last
interpreter, if this option isn't given. So either this test has to set
the "needs_destroy" on the IO object or IO has a bug.
If there are objects that did "needs_destroy" then a lazy sweep is done,
which should flush the IO object or release resources or whateve rsuch
an object might need.
Destroying the last interpreter can be very time consuming, when
millions of objects are allocated. I don't think this should be the
default.
Note: only the last interpreter is effected - all other interpreters
allways clean up behind them.
> Dan
leo
Dan Sugalski
No. If any object has a destructor it should be called as the last
interpreter is shut down. We're not guaranteeing dead-on immediate
destruction, or if the timely flag isn't set timely destruction, but we
*are* guaranteeing eventual destruction.
We don't need to free memory, but we do need to call destructors.
Dan
Juergen Boemmels
> > I disagree :) We already have a 2 stage IO destroy. The first shall
> > flush its files. This get called even if destroy-at-end isn't set.
The two stage IO destroy is a problem that we don't have destruction
ordering yet. Closing the standard handles to early leads to failed tests.
> > The
> > second phase (clean allocated memory) isn't called for the last
> > interpreter, if this option isn't given. So either this test has to set
> > the "needs_destroy" on the IO object or IO has a bug.
No, needs_destroy marks the object that it needs _early_ destruction,
not that it needs destruction at all. The problem that the
destruction code is not run run at all.
> > If there are objects that did "needs_destroy" then a lazy sweep is done,
> > which should flush the IO object or release resources or whateve rsuch
> > an object might need.
>
> No. If any object has a destructor it should be called as the last
> interpreter is shut down. We're not guaranteeing dead-on immediate
> destruction, or if the timely flag isn't set timely destruction, but we
> *are* guaranteeing eventual destruction.
>
> We don't need to free memory, but we do need to call destructors.
Exactly. My first idea --destroy-at-end only removed the symptom. The
real problem was that the final sweep, which should have run the
destructor code, was only triggered if there was still an object left
which needs early destruction. I changed this to run the final sweep
unconditionally and the problem disappeared. I did not touch any of
the --destroy-at-end semantics.
I thought one moment about introducing a variable
interpreter->has_destroy analog to interpreter->has_early_DOD_PMCs but
rejected it. This would slow down the DOD because it has to take care
of this variable also. I think its better to force one sweep at the
end than slowing down every DOD.
Leopold Toetsch
> It turned out, that it was even simpler. The explicit sweeping is
> already in the code, but is only triggered if there are objects which
> need _early_ destruction, not if objects just need
> destruction. Removing this test made it work.
Why can't you call PIO_finish(DO_FLUSH) before the sweep? I don't
understand these workarounds. Also you mark the IOdata live, only that
they are destroyed 3 lines later then.
If there are no objects that need timely destruction, the sweep is wasted
time.
> Fix is commited.
Not IMHO.
PIO_finish still does not release the standard handles and valgrind
complains about invalid reads in PIO_flush_down.
> cu
> boe
leo
Leopold Toetsch
> No. If any object has a destructor it should be called as the last
> interpreter is shut down. We're not guaranteeing dead-on immediate
> destruction, or if the timely flag isn't set timely destruction, but we
> *are* guaranteeing eventual destruction.
Oha. That explains the reasoning. That adds a third category to
"destruction" IMHO:
1) timely destruction on scope exits
2) eventual destruction on exit of interpreters
3) memory destruction for leak tests and such or for intermediate
interpreters.
For 1) we have opcode support C<needs_destroy>. 2) seems to be the case
where IO objects jump in - and HLL destructors.
> We don't need to free memory, but we do need to call destructors.
So we'd better separate 2) and 3) This would simplify destruction
ordering and speedup interpreter shutdown.
> Dan
leo
Juergen Boemmels
> Juergen Boemmels <boem...@physik.uni-kl.de> wrote:
>
> > It turned out, that it was even simpler. The explicit sweeping is
> > already in the code, but is only triggered if there are objects which
> > need _early_ destruction, not if objects just need
> > destruction. Removing this test made it work.
>
> Why can't you call PIO_finish(DO_FLUSH) before the sweep?
Because the object in question is not in PIO_data. Its just a plain
PMC with a destroy-function. And this destroy-function was not run
until now. Some other bug was hiding that fact.
> I don't
> understand these workarounds. Also you mark the IOdata live, only that
> they are destroyed 3 lines later then.
Thats something completely different and I addmit it is a hack. When I
introduced this, I had problems with closing the three _standard_
handles to early. Maybe I don't need this hack any more because the
handles are now only flushed (not tested). But I left it in because
its a matter of correctness. One destroy-function which prints and the
problem is there again. The correct solution to this problem is
destruction ordering: The standard handles should be destroyed after
the interpreter (which isn't a PMC yet).
> If there are no objects that need timely destruction, the sweep is wasted
> time.
>
> > Fix is commited.
>
> Not IMHO.
> PIO_finish still does not release the standard handles and valgrind
> complains about invalid reads in PIO_flush_down.
I think this is one (or two) other problems. When I think about it, I
just flush the buffers, but do not free them. Will look at the code now.
bye
Juergen Boemmels
> Dan Sugalski <d...@sidhe.org> wrote:
>
> > No. If any object has a destructor it should be called as the last
> > interpreter is shut down. We're not guaranteeing dead-on immediate
> > destruction, or if the timely flag isn't set timely destruction, but we
> > *are* guaranteeing eventual destruction.
>
> Oha. That explains the reasoning. That adds a third category to
> "destruction" IMHO:
>
> 1) timely destruction on scope exits
> 2) eventual destruction on exit of interpreters
or any time between the death of the object and the and of
universe^H^H^Hinterpreter. But no guarantee when.
> 3) memory destruction for leak tests and such or for intermediate
> interpreters.
>
> For 1) we have opcode support C<needs_destroy>. 2) seems to be the case
> where IO objects jump in - and HLL destructors.
>
> > We don't need to free memory, but we do need to call destructors.
>
> So we'd better separate 2) and 3) This would simplify destruction
> ordering and speedup interpreter shutdown.
The code is already there. The final sweep runs the destructors. The
problem was that 1) and 2) were to strongly coupled. All destructors
were only run if one object exists which needed timely destruction.
Maybe we should generate another PMC which uses destruction functions,
just for testing these things.
Dan Sugalski
Sure, that works. We do need to be able to free up all an interpreter's
allocated memory, so we don't leak when we're embedded in some other
application, but running the destructors is definitely more important.
Dan