FastStack and GC

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

FastStack and GC

elifarley
The methods 'pop' and 'popSilently' from class 'FastStack' don't
nullify the reference to the object being popped, which prevents them
from being garbage collected.

So, instead of

    public void popSilently() {
      this.pointer--;
    }

    public Object pop() {
      return this.stack[--this.pointer];
    }

I suggest this:

    public void popSilently() {
      this.stack[--this.pointer] = null;
    }

    public Object pop() {
      final Object result = this.stack[--this.pointer];
      this.stack[this.pointer] = null;
      return result;
    }


Or is there a reason not to do so ? Setting to null does take more
time than doing nothing, but I think the added overhead is negligible
in face of a better garbage collection.

What do you think ?
Reply | Threaded
Open this post in threaded view
|

Re: FastStack and GC

Rickard Öberg
Elifarley Callado Coelho Cruz wrote:

> The methods 'pop' and 'popSilently' from class 'FastStack' don't
> nullify the reference to the object being popped, which prevents them
> from being garbage collected.
>
> So, instead of
>
>     public void popSilently() {
>       this.pointer--;
>     }
>
>     public Object pop() {
>       return this.stack[--this.pointer];
>     }
>
> I suggest this:
>
>     public void popSilently() {
>       this.stack[--this.pointer] = null;
>     }
>
>     public Object pop() {
>       final Object result = this.stack[--this.pointer];
>       this.stack[this.pointer] = null;
>       return result;
>     }
I may be unusually dense today, but shouldn't that be
this.stack[this.pointer--] = null;
i.e. set it to null first and *then* decrease the counter. Otherwise it
will be removing things that should still be on the stack. Similarly,
pop() should probably be:
 >     public Object pop() {
 >       final Object result = this.stack[this.pointer];
 >       this.stack[this.pointer--] = null;
 >       return result;
 >     }


/Rickard

--
Rickard Öberg
[hidden email]
@work +46-(0)19-173036
Reply | Threaded
Open this post in threaded view
|

RE: FastStack and GC

Jörg Schaible
In reply to this post by elifarley
Rickard Öberg wrote on Monday, January 09, 2006 1:21 PM:

> Elifarley Callado Coelho Cruz wrote:
>> The methods 'pop' and 'popSilently' from class 'FastStack' don't
>> nullify the reference to the object being popped, which prevents
>> them from being garbage collected.
>>
>> So, instead of
>>
>>     public void popSilently() {
>>       this.pointer--;
>>     }
>>
>>     public Object pop() {
>>       return this.stack[--this.pointer];
>>     }
>>
>> I suggest this:
>>
>>     public void popSilently() {
>>       this.stack[--this.pointer] = null;
>>     }
>>
>>     public Object pop() {
>>       final Object result = this.stack[--this.pointer];
>>       this.stack[this.pointer] = null;
>>       return result;
>>     }
>
> I may be unusually dense today, but shouldn't that be
> this.stack[this.pointer--] = null;
> i.e. set it to null first and *then* decrease the counter.

<g> ... That was my first reaction also :)

> Otherwise it
> will be removing things that should still be on the stack. Similarly,
>  pop() should probably be: >     public Object pop() {
>  >       final Object result = this.stack[this.pointer];
>  >       this.stack[this.pointer--] = null;
>  >       return result;
>  >     }
> /Rickard

Elifarley, can you open a JIRA issue?

- Jörg
Reply | Threaded
Open this post in threaded view
|

Re: FastStack and GC

elifarley
Let's examine the 'push' method:

...
        this.stack[this.pointer++] = value;
...

The initial value of this.pointer is 0, so at the first time the
'push' method is called, we have:

this.stack[0] = value;

and then this.pointer becomes 1.

If the 'pop' method is called immediately after the first 'push'
method, we will have to return the element at position 0 and then we
have to nullify the same position 0.

Following your suggestion, we will have:

public Object pop() {
 final Object result = this.stack[this.pointer];
 this.stack[this.pointer--] = null;
 return result;
}

Since  this.pointer has previously been increased to 1, the first line
is equivalent (at the present hypotetical situation) to:

 final Object result = this.stack[1];

Or, more precisely :

 final Object result = null;

for the second line we have:

 this.stack[this.pointer--] = null;

which is equivalent to:

 this.stack[1] = null;

and then this.pointer becomes 0;

But the reference at stack[1] was already null.

And finally:

 return result;

which is equivalent to:

return null;

Now lets see how the original proposition would work:

      final Object result = this.stack[--this.pointer];

since this.pointer is currently at 1, we have:

      final Object result = this.stack[0]; // stack[0] holds the
previously pushed reference.
and now this.pointer becomes 0.

      this.stack[this.pointer] = null;
which is:
      this.stack[0] = null;

and finally:

      return result; // which effectively returns the reference
previously pushed.

Hope that clears things out :)

On 1/9/06, Jörg Schaible <[hidden email]> wrote:

> Rickard Öberg wrote on Monday, January 09, 2006 1:21 PM:
>
> > Elifarley Callado Coelho Cruz wrote:
> >> The methods 'pop' and 'popSilently' from class 'FastStack' don't
> >> nullify the reference to the object being popped, which prevents
> >> them from being garbage collected.
> >>
> >> So, instead of
> >>
> >>     public void popSilently() {
> >>       this.pointer--;
> >>     }
> >>
> >>     public Object pop() {
> >>       return this.stack[--this.pointer];
> >>     }
> >>
> >> I suggest this:
> >>
> >>     public void popSilently() {
> >>       this.stack[--this.pointer] = null;
> >>     }
> >>
> >>     public Object pop() {
> >>       final Object result = this.stack[--this.pointer];
> >>       this.stack[this.pointer] = null;
> >>       return result;
> >>     }
> >
> > I may be unusually dense today, but shouldn't that be
> > this.stack[this.pointer--] = null;
> > i.e. set it to null first and *then* decrease the counter.
>
> <g> ... That was my first reaction also :)
>
> > Otherwise it
> > will be removing things that should still be on the stack. Similarly,
> >  pop() should probably be: >     public Object pop() {
> >  >       final Object result = this.stack[this.pointer];
> >  >       this.stack[this.pointer--] = null;
> >  >       return result;
> >  >     }
> > /Rickard
>
> Elifarley, can you open a JIRA issue?
>
> - Jörg
>
Reply | Threaded
Open this post in threaded view
|

Re: FastStack and GC

Rickard Öberg
Hi!

Elifarley Callado Coelho Cruz wrote:

> Now lets see how the original proposition would work:
>
>       final Object result = this.stack[--this.pointer];
>
> since this.pointer is currently at 1, we have:
>
>       final Object result = this.stack[0]; // stack[0] holds the
> previously pushed reference.
> and now this.pointer becomes 0.
>
>       this.stack[this.pointer] = null;
> which is:
>       this.stack[0] = null;
>
> and finally:
>
>       return result; // which effectively returns the reference
> previously pushed.
>
> Hope that clears things out :)
Indeed! It confirms my suspicion that I am particularly dense today.
Thanks for that concise description :-)

regards,
   Rickard

--
Rickard Öberg
[hidden email]
@work +46-(0)19-173036
Reply | Threaded
Open this post in threaded view
|

Re: FastStack and GC

elifarley
In reply to this post by Jörg Schaible
Jira issue url:
http://jira.codehaus.org/browse/XSTR-264


On 1/9/06, Jörg Schaible <[hidden email]> wrote:

> Rickard Öberg wrote on Monday, January 09, 2006 1:21 PM:
>
> > Elifarley Callado Coelho Cruz wrote:
> >> The methods 'pop' and 'popSilently' from class 'FastStack' don't
> >> nullify the reference to the object being popped, which prevents
> >> them from being garbage collected.
> >>
> >> So, instead of
> >>
> >>     public void popSilently() {
> >>       this.pointer--;
> >>     }
> >>
> >>     public Object pop() {
> >>       return this.stack[--this.pointer];
> >>     }
> >>
> >> I suggest this:
> >>
> >>     public void popSilently() {
> >>       this.stack[--this.pointer] = null;
> >>     }
> >>
> >>     public Object pop() {
> >>       final Object result = this.stack[--this.pointer];
> >>       this.stack[this.pointer] = null;
> >>       return result;
> >>     }
> >
> > I may be unusually dense today, but shouldn't that be
> > this.stack[this.pointer--] = null;
> > i.e. set it to null first and *then* decrease the counter.
>
> <g> ... That was my first reaction also :)
>
> > Otherwise it
> > will be removing things that should still be on the stack. Similarly,
> >  pop() should probably be: >     public Object pop() {
> >  >       final Object result = this.stack[this.pointer];
> >  >       this.stack[this.pointer--] = null;
> >  >       return result;
> >  >     }
> > /Rickard
>
> Elifarley, can you open a JIRA issue?
>
> - Jörg
>