mariano could you update the comments to reflect this discussion. I would like that the next guy that looks at the code can learn and not be puzzled.
Stef On Jan 9, 2012, at 7:42 PM, Mariano Martinez Peck wrote: > > > On Mon, Jan 9, 2012 at 7:13 PM, Levente Uzonyi <[email protected]> wrote: > On Mon, 9 Jan 2012, Mariano Martinez Peck wrote: > > Hi Levente. Thanks for looking into the issue. I saw your code and there is > something I don't understand. > > pointsTo: anObject > "Answers true if the garbage collector would fail to collect anObject > because I hold a reference to it, or false otherwise" > > (self instVarsInclude: anObject) > ifTrue: [ > self class isWeak ifFalse: [ ^true ]. > 1 to: self class instSize do: [ :i | > (self instVarAt: i) == anObject ifTrue: [ ^true ] ]. > ^false ] > ifFalse: [ ^self class == anObject and: [ self class isCompact not > ] ] > > > I don't understand the loop of > > 1 to: self class instSize do: [ :i | > (self instVarAt: i) == anObject ifTrue: [ ^true ] ]. > > > In which scenario can (self instVarsInclude: anObject) answer true, > but the loop false? > > The scenario happens when the receiver has weak slots and the argument is > referenced from one of those weak slots, but not from the other slots. > > > Ok, I see. And moreover, there has to be a GC in the middle, right? so..the > scenario is "The scenario happens when the receiver has weak slots, the > argument is referenced from one of those weak slots, but not from the other > non-weak slots, and also when a GC runs between the invokation to > #instVarsInclude: and the loop". > is that correct? so that I will add this comment to the code ;) > > > doesn't #instVarsInclude do exactly what you are doing there? > > Just partially. Since we have no information about which slots hold the > reference to the argument, therefore this loop must be "repeated". > > Ok, I see. > > > > > Anyway, I have integrated your changes in Pharo, but still, I have the same > problem :( > If I understand correctly, the following shouldn't fail, but it does. Here > is the version of Squeak that fails. > > The assetion fails, because the indirection vector (the Array found by > PointerFinder) holds a reference to the object after the first assignment to > a. If you move the temporary inside the block or use an inlined loop (e.g. > #to:do: with literal block argument), then the assertion won't fail. So this > is just a normal (maybe surprising) reference to the object. > > > Excellent. Now I got it. Thank you very much for your help Levente. Now > PointerFinderTest are green :) > > > Levente > > > | a | > 10 timesRepeat: [ > a := Date new. > Smalltalk garbageCollect. > self assert: (PointerFinder pointersTo: a) isEmpty > ] > > Thanks a lot, > > > On Mon, Jan 9, 2012 at 2:40 PM, Levente Uzonyi <[email protected]> wrote: > > On Sun, 8 Jan 2012, Mariano Martinez Peck wrote: > > What I don't understand is why in Squeak it does work. > > > > Because #pointsTo: is not used in Squeak (yet). As usual I dug deeper > than > I should have, so I'll publish a few changes soon. > > > Ok, you are right. Squeak #inboundPointersExcluding: is using > #instVarsInclude: rather than #pointsTo. And that solves the problem in > Pharo as well. But still, I would like to understand why we get those > method contexts with #pointsTo. > > > Because #pointsTo: is a normal message send, it even sends other methods, > so it will create contexts. > > > Thanks Levente for your help. If you find something let us know, I want to > learn :) > > > I pushed my changes to the Squeak Inbox, which fully works around this > issue. The changes about weak references can simply be removed if you don't > like them, the rest will just work without them. > > > Levente > > > Thanks > > > > Levente > > > Thanks in advance Levente! > > > > > > it will create at least one new MethodContext which is not included in > > that list. > > > Levente > > > > > > > > Levente > > > > Thanks again. > > > > > Levente > > > > > > Do you mean what I understand :)? that some tools compiled > methods? > :) > > > > Stef > > > > > > -- > > Mariano > http://marianopeck.wordpress.**********com < > http://marianopeck.wordpress. > **** > com <http://marianopeck.wordpress.******com<http://marianopeck.** > wordpress.com > <http://marianopeck.wordpress.**com<http://marianopeck.wordpress.com> > > > > > > > > -- > > Mariano > http://marianopeck.wordpress.********com < > http://marianopeck.wordpress. > **** > com <http://marianopeck.wordpress.****com<http://marianopeck.** > wordpress.com <http://marianopeck.wordpress.com>> > > > > > > > -- > Mariano > http://marianopeck.wordpress.******com <http://marianopeck.wordpress. > **** > com <http://marianopeck.wordpress.**com<http://marianopeck.wordpress.com> > > > > > > > -- > Mariano > http://marianopeck.wordpress.****com <http://marianopeck.wordpress.** > com <http://marianopeck.wordpress.com>> > > > > > > -- > Mariano > http://marianopeck.wordpress.**com <http://marianopeck.wordpress.com> > > > > > > -- > Mariano > http://marianopeck.wordpress.com > > > > > > -- > Mariano > http://marianopeck.wordpress.com >
