w should be made non-recursive before doing anything else--otherwise we
would have to keep track of whether it is or not during the loop. So
adding it to the stack just increments the reference count, and popping
it decrements it again. No danger of messing with child reference counts
or anything.
I made the changes to the two files I mentioned, and naturally there are
still test failures beyond those with no-op gc. They shouldn't take too
long to work out, though. I'm tired enough of actually reading code that
I think I'll just do a binary search on instances of gc...
Marshall
On Mon, May 23, 2016 at 03:59:25PM -0400, Henry Rich wrote:
> You are right, provided you are sure w is off the stack. If you can
> be sure of that, it's better to derec the child as you add it.
>
> I also answered my question about the use of jt->tbase.
>
> Henry
>
> On 5/23/2016 1:56 PM, Marshall Lochbaum wrote:
> >I don't see why tpopnotw would be any better than just leaving w off the
> >stack (or, perhaps, why leaving w off the stack is flawed). The only
> >reason these functions break our current system is that they add a
> >recursive object as a child of a non-recursive one. So calling derec on
> >the children solves the problem, and any other changes are for
> >performance. And tpopexcept is clearly going to be slower than just
> >tpop. So what's the benefit?
> >
> >Marshall
> >
> >On Sun, May 22, 2016 at 12:17:19PM -0400, Henry Rich wrote:
> >>no, tpopnotw would not leave the w dangling - it would move it (see
> >>my latest). It would work like this (you will have to reflect any
> >>changes you made to tpop/tpush):
> >>
> >>tpopexcept(old,w){
> >> I wfound = 0; // set if there was a free of w
> >> while(old<jt->tbase+jt->ttop){ // till we have freed back to 'old'
> >> //?? question: why is 'old <' used rather than 'old !='? Aren't the
> >> // blocks of the stack in accidental order, so that this test
> >>would fail
> >> // if the stack spans a block boundary, and the second block is at a
> >> // lower address than the first?
> >> if(1<jt->ttop) { // if the stack item is a pointer to a free block
> >> A blocktofree = jt->tstack[--jt->ttop]; // fetch address to free
> >> if(blocktofree==w)++wfound;else fr(blocktofree); // free it,
> >>unless it's w
> >> elsetf(); // stack item is a stack-block chain pointer - free
> >>the stack block
> >> }
> >> DO(wfound, tpush(w)); // restore any occurrences of w that were
> >>suppressed
> >> R old; // return free-to pointer - it may not be the top, if w was
> >>restored
> >>}
> >>
> >>I agree that if it's w being changed, nothing special is needed, but
> >>this would be a good idea for the other places.
> >>
> >>What testcase would you like me to look at first?
> >>
> >>Henry
> >>
> >>
> >>On 5/22/2016 11:52 AM, Marshall Lochbaum wrote:
> >>>Replying to both of your comments here.
> >>>
> >>>Almost all uses of this pattern actually replace w rather than append to
> >>>it. I haven't tested, but I think it's fine in all of these cases to use
> >>>gc or gc3. The exceptions which do modify w are in cp.c and vgauss.c,
> >>>although it's possible I missed some.
> >>>
> >>>tpopnotw isn't possible--the stack is stored in an array, so ignoring w
> >>>would just leave it dangling beyond the end of the stack. Your first
> >>>suggestion still works, except that derec, rather than ra, makes objects
> >>>non-recursive.
> >>>
> >>>I've pushed my changes so far to the unbox branch refcount:
> >>>https://github.com/iocane/unbox/commits/refcount
> >>>
> >>>Failing tests are
> >>>
> >>>- Because 7!:0 is no longer accurate
> >>> core/g202.ijs
> >>> core/g202b.ijs
> >>> core/g7x.ijs
> >>> core/glocale.ijs
> >>>
> >>>- Problem with in-place sparse amends
> >>> sparse/gsp530i.ijs
> >>> sparse/gsp530l.ijs
> >>> sparse/gsp530n.ijs
> >>>
> >>>- Miscellaneous
> >>> core/g13x.ijs
> >>> core/g5x2.ijs
> >>> core/gq.ijs
> >>>
> >>>Presumably the last two groups are use-after-frees. Standard tools don't
> >>>work to identify these because of J's pool allocation, and making J use
> >>>mallocs instead of pool allocations causes bugs for reasons that are
> >>>probably even harder to track down. I have tried to configure Valgrind
> >>>to recognize pool allocations (using the requests described at
> >>>http://www.network-theory.co.uk/docs/valgrind/valgrind_57.html), but it
> >>>seems to only recognize that certain reads/writes are invalid, not the
> >>>free that caused them to be invalid. Useless.
> >>>
> >>>Marshall
> >>>
> >>>On Sat, May 21, 2016 at 04:29:03PM -0400, Henry Rich wrote:
> >>>> store the current stack top in variable old.
> >>>> for(...) {
> >>>> add something to boxed array w.
> >>>> call gc(w,old), putting w in non-recursive form.
> >>>> }
> >>>>
> >>>>What I was suggesting is:
> >>>>
> >>>> store the current stack top in variable old.
> >>>> ra(w);
> >>>> for(...) {
> >>>> add something to boxed array w.
> >>>> ra(something); // putting something in non-recursive form.
> >>>> tpop(old);
> >>>> }
> >>>> tpush(w);
> >>>>
> >>>>Now I suggest
> >>>>
> >>>> store the current stack top in variable old.
> >>>> for(...) {
> >>>> add something to boxed array w.
> >>>> ra(something); // putting something in non-recursive form?
> >>>> tpopnotw(old,w);
> >>>> }
> >>>>
> >>>>Where tpopnotw is like tpop but simply ignores stack entries that match w.
> >>>>
> >>>>If this idea is sound, I think you might as well do it, because
> >>>>
> >>>>* you will have to inspect each place gc() is used anyway
> >>>>* assuming that the current code is there because the free was needed,
> >>>> this preserves that behavior. It will be hard to be confident that
> >>>> this is not needed (you'd have to deploy and wait for user complaints
> >>>> -
> >>>> I wouldn't want to take that chance)
> >>>>* No usecount will be modified or block freed that doesn't need to be
> >>>>* it is necessary to increment the usecount of (something)
> >>>>
> >>>>Henry
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>On 5/20/2016 10:06 AM, Marshall Lochbaum wrote:
> >>>>>>>A number of spots in the J source follow this pattern, or similar:
> >>>>>>> store the current stack top in variable old.
> >>>>>>> for(...) {
> >>>>>>> add something to boxed array w.
> >>>>>>> call gc(w,old), putting w in non-recursive form.
> >>>>>>> }
> >>>>>>>
> >>>>>>>(A particular example is cp.c, lines 91--97, which performs (v^:l x)
> >>>>>>>where l is an integer list).
> >>>>----------------------------------------------------------------------
> >>>>For information about J forums see http://www.jsoftware.com/forums.htm
> >>>----------------------------------------------------------------------
> >>>For information about J forums see http://www.jsoftware.com/forums.htm
> >>----------------------------------------------------------------------
> >>For information about J forums see http://www.jsoftware.com/forums.htm
> >----------------------------------------------------------------------
> >For information about J forums see http://www.jsoftware.com/forums.htm
>
> ----------------------------------------------------------------------
> For information about J forums see http://www.jsoftware.com/forums.htm
----------------------------------------------------------------------
For information about J forums see http://www.jsoftware.com/forums.htm
