A possible Cog bug (was: Re: [squeak-dev] Direct implementation of shift/reset in Smalltalk)
On Thu, 21 Apr 2011, Frank Shearar wrote:
I finished a preliminary version of the shift/reset control operator for creating and using partial continuations:
http://www.squeaksource.com/Control.html
It's pretty easy to use:
snip
Comments welcome!
I thought it's a bit easier to use notifications instead of control operators. Here's an example:
c := [ 3 + PartialContinuationNotification signal ] on: PartialContinuationNotification do: [ :not | not continuation ]. c value: 4. "==> 7"
So I implemented it and uploaded it to http://leves.web.elte.hu/squeak/Control-ul.5.mcz . I also refactored the code a bit. Even though some Pharo folks always wanted to be able to use streams with OrderedCollection (which works in Squeak btw), it's kinda pointless thing to use #streamContents: with them, because OrderedCollections can grow by themselves, unlike Arrays or Strings. The code works, the tests are green, etc.
But I wasn't happy with the results, because ContextPart already implements #copyTo: (which I think can also be simplified by removing BlockContext support), which can be used to serialize the stack. So I wrote the code, but it didn't work for some reason. The execution always ran into the error 'computation has been terminated' in MethodContext >> #cannotReturn: and the method context (which stitches the continuation onto the stack) was broken (nil in temporaries where should be non nil values). Finally I tried it in SqueakVM instead of Cog and it worked. The tests are green, the example code works, etc. The code with these changes is available at http://leves.web.elte.hu/squeak/Control-ul.6.mcz . I prepared an image to ease the debugging process, which is available at http://leves.web.elte.hu/squeak/ContextBug.zip .
Levente
P.S.: Feel free to add an issue to the Cog tracker. :)
frank
[1] http://www.lshift.net/blog/2011/04/20/direct-implementation-of-shiftreset-in...
On Sun, 24 Apr 2011, Levente Uzonyi wrote:
snip
it didn't work for some reason. The execution always ran into the error 'computation has been terminated' in MethodContext >> #cannotReturn: and the method context (which stitches the continuation onto the stack) was broken (nil in temporaries where should be non nil values). Finally I tried it in SqueakVM instead of Cog and it worked. The tests are green, the example code
I tracked down this issue a bit more. The cause of the problem is that with CogVM the pc of thisContext is being set to nil when the execution leaves the context. This means, that [thisContext copy] will not be able to copy the pc of the context:
{ thisContext pc. thisContext copy pc } "==> #(23 nil)"
Levente
On Sun, Apr 24, 2011 at 8:35 PM, Levente Uzonyi leves@elte.hu wrote:
On Sun, 24 Apr 2011, Levente Uzonyi wrote:
snip
it didn't work for some reason. The execution always ran into the error
'computation has been terminated' in MethodContext >> #cannotReturn: and the method context (which stitches the continuation onto the stack) was broken (nil in temporaries where should be non nil values). Finally I tried it in SqueakVM instead of Cog and it worked. The tests are green, the example code
I tracked down this issue a bit more. The cause of the problem is that with CogVM the pc of thisContext is being set to nil when the execution leaves the context. This means, that [thisContext copy] will not be able to copy the pc of the context:
{ thisContext pc. thisContext copy pc } "==> #(23 nil)"
Great catch Levente. I'll try and track this down asap.
Levente
On Sun, 24 Apr 2011, Eliot Miranda wrote:
Great catch Levente. I'll try and track this down asap.
Great, thanks. I found that primitive 148 (#shallowCopy) doesn't copy the pc and the temporaries (indexable fields). But there are some other interesting cases, where the number of indexable fields are different. To reproduce this issue, replace ContextPart >> #copyTo: with the following:
copyTo: aContext "Copy self and my sender chain down to, but not including, aContext. End of copied chain will have nil sender."
| copy | self == aContext ifTrue: [ ^nil ]. copy := self copy. "Cog doesn't copy the pc, so copy it here." copy pc: pc. copy basicSize = self basicSize ifFalse: [ self halt ]. sender ifNotNil: [ copy privSender: (sender copyTo: aContext) ]. ^copy
Then evaluate
c := [ 3 + [ :k | k ] shift ] reset.
in the workspace in the image I prepared. Two halts will appear after each other (proceed after the first). In the first case the size of the copy is smaller than the original, in the second case it's larger.
Levente
Hi Levente,
try out the latest VMs. They don't fix the bugs, but do change the symptoms. e.g. in
c := [ 3 + PartialContinuationNotification signal ] on: PartialContinuationNotification do: [ :not | not continuation ]. c value: 4. "==> 7"
there is now an MNU of #+ since 3 has been replaced by a MethodContext.
So if I could ask you could dig in again, that would help.
thanks!
best, Eliot
2011/4/25 Levente Uzonyi leves@elte.hu
On Sun, 24 Apr 2011, Eliot Miranda wrote:
Great catch Levente. I'll try and track this down asap.
Great, thanks. I found that primitive 148 (#shallowCopy) doesn't copy the pc and the temporaries (indexable fields). But there are some other interesting cases, where the number of indexable fields are different. To reproduce this issue, replace ContextPart >> #copyTo: with the following:
copyTo: aContext "Copy self and my sender chain down to, but not including, aContext. End of copied chain will have nil sender."
| copy | self == aContext ifTrue: [ ^nil ]. copy := self copy. "Cog doesn't copy the pc, so copy it here." copy pc: pc. copy basicSize = self basicSize ifFalse: [ self halt ]. sender ifNotNil: [ copy privSender: (sender copyTo: aContext) ]. ^copyThen evaluate
c := [ 3 + [ :k | k ] shift ] reset.
in the workspace in the image I prepared. Two halts will appear after each other (proceed after the first). In the first case the size of the copy is smaller than the original, in the second case it's larger.
Levente
Hi Eliot,
On Tue, 26 Apr 2011, Eliot Miranda wrote:
Hi Levente, try out the latest VMs. They don't fix the bugs, but do change the symptoms. e.g. in
c := [ 3 + PartialContinuationNotification signal ] on: PartialContinuationNotification do: [ :not | not continuation ]. c value: 4. "==> 7"
there is now an MNU of #+ since 3 has been replaced by a MethodContext.
So if I could ask you could dig in again, that would help.
the symptoms are much better now. The cause of the MNU seems to be that the copied contexts have some "junk" at their indexable slots. Implementing ContextPart >> #copyTo: as below solves (hides) the problem.
ContextPart >> copyTo: aContext "Copy self and my sender chain down to, but not including, aContext. End of copied chain will have nil sender."
| copy | self == aContext ifTrue: [ ^nil ]. copy := self copy. "Cog doesn't copy the temps properly, so copy them here." 1 to: self size do: [ :i | copy at: i put: (self at: i) ]. sender ifNotNil: [ copy privSender: (sender copyTo: aContext) ]. ^copy
To reproduce the "junk" copying bug, evaluate the following:
| context | context := thisContext copy. { context. context copy } explore
The first context's first slot will be nil, while the second context's first slot will point to the sender context. Actually the indexable slots are filled with the values of the fixed slots, that's why the sender context is at the first indexable slot of the second context. Here's an example with more indexable slots:
| context a b c d e | context := thisContext copy. a := b := c := d := e := context. { context. context copy } explore. { a. b. c. d. e } "This is here to avoid compiler complaints."
Levente
thanks!
best, Eliot
Hi Levente,
2011/4/26 Levente Uzonyi leves@elte.hu
Hi Eliot,
On Tue, 26 Apr 2011, Eliot Miranda wrote:
Hi Levente,
try out the latest VMs. They don't fix the bugs, but do change thesymptoms. e.g. in
c := [ 3 + PartialContinuationNotification signal ] on: PartialContinuationNotification do: [ :not | not continuation ]. c value: 4. "==> 7"
there is now an MNU of #+ since 3 has been replaced by a MethodContext.
So if I could ask you could dig in again, that would help.
the symptoms are much better now. The cause of the MNU seems to be that the copied contexts have some "junk" at their indexable slots. Implementing ContextPart >> #copyTo: as below solves (hides) the problem.
ContextPart >> copyTo: aContext "Copy self and my sender chain down to, but not including, aContext. End of copied chain will have nil sender."
| copy | self == aContext ifTrue: [ ^nil ]. copy := self copy. "Cog doesn't copy the temps properly, so copy them here." 1 to: self size do: [ :i | copy at: i put: (self at: i) ]. sender ifNotNil: [ copy privSender: (sender copyTo: aContext) ]. ^copyTo reproduce the "junk" copying bug, evaluate the following:
| context | context := thisContext copy. { context. context copy } explore
The first context's first slot will be nil, while the second context's first slot will point to the sender context. Actually the indexable slots are filled with the values of the fixed slots, that's why the sender context is at the first indexable slot of the second context. Here's an example with more indexable slots:
| context a b c d e | context := thisContext copy. a := b := c := d := e := context. { context. context copy } explore. { a. b. c. d. e } "This is here to avoid compiler complaints."
Hmm... the above behavior is intentional and correct in that to speed up context-to-stack mapping Cog makes no guarantee to preserve the values of temporary values after a context has returned. You'll notice that the entire closure scheme is centered around this, in that temporaries that must outlive their dynamic extent for the correct working of blocks are either copied into blocks (as copied values) or stored into remote temp vectors, hence breaking any dependence on the temporaries in enclosing contexts.
If you can live with this restriction then instead of stating "Cog doesn't copy the temps properly, so copy them here." in your reimplementation of copyTo: it would be more accurate to state "Cog doesn't preserve non-closed-over temps beyond their dynamic extent, so copy them here." or some such.
Note that Cog /does/ preserve arguments. However, I've clearly still got some bug because look at the following. In the copy the arguments ba1 & ba2 (1 & 2) are in the wrong place on the stack. Sigh...
[:ba1 :ba2| | context a b c d e | context := thisContext copy. a := b := c := d := e := context. { context. context copy } explore. { a. b. c. d. e } "This is here to avoid compiler complaints."] value: 1 value: 2
Levente
thanks!
best, Eliot
Hi Eliot,
On Tue, 26 Apr 2011, Eliot Miranda wrote:
Hmm... the above behavior is intentional and correct in that to speed up context-to-stack mapping Cog makes no guarantee to preserve the values of temporary values after a context has returned. You'll notice that the entire closure scheme is centered around this, in that temporaries that must outlive their dynamic extent for the correct working of blocks are either copied into blocks (as copied values) or stored into remote temp vectors, hence breaking any dependence on the temporaries in enclosing contexts.
it's probably just the lack of my knowledge about VMs, but I don't see why an existing object's slots can't be copied by primitive 148. Here's an example:
| context | context := MethodContext newForMethod: Object >> #yourself. context stackp: 3. 1 to: 3 do: [ :index | context at: index put: index ]. { context. context copy } explore
The forged contexts in the above example never returned (because they weren't executed), but the copied context lacks the temps of the original.
If you can live with this restriction then instead of stating "Cog doesn't copy the temps properly, so copy them here." in your reimplementation of copyTo: it would be more accurate to state "Cog doesn't preserve non-closed-over temps beyond their dynamic extent, so copy them here." or some such.
That's acceptable, but I think that loop is superfluous, see below.
Note that Cog /does/ preserve arguments. However, I've clearly still got some bug because look at the following. In the copy the arguments ba1 & ba2 (1 & 2) are in the wrong place on the stack. Sigh...
[:ba1 :ba2| | context a b c d e | context := thisContext copy. a := b := c := d := e := context. { context. context copy } explore. { a. b. c. d. e } "This is here to avoid compiler complaints."] value: 1 value: 2
I think that this bug causes the problem with both #copyTo: and the above example. When this is fixed, then there will be no need to copy the temps slots in #copyTo:. The temp slots are copied in the next example, but they are off by 6 (the number of fixed slots):
| context | context := MethodContext newForMethod: Object >> #yourself. context stackp: 16. 1 to: 3 do: [ :index | context at: index put: index ]. { context. context copy} explore
Cheers, Levente
Levente
thanks!
best, Eliot
On 2011/04/25 04:35, Levente Uzonyi wrote:
On Sun, 24 Apr 2011, Levente Uzonyi wrote:
snip
it didn't work for some reason. The execution always ran into the error 'computation has been terminated' in MethodContext >> #cannotReturn: and the method context (which stitches the continuation onto the stack) was broken (nil in temporaries where should be non nil values). Finally I tried it in SqueakVM instead of Cog and it worked. The tests are green, the example code
I tracked down this issue a bit more. The cause of the problem is that with CogVM the pc of thisContext is being set to nil when the execution leaves the context. This means, that [thisContext copy] will not be able to copy the pc of the context:
{ thisContext pc. thisContext copy pc } "==> #(23 nil)"
That would explain why I didn't come across it: I wrote the library on the SqueakVM.
frank
On Mon, 25 Apr 2011, Frank Shearar wrote:
On 2011/04/25 04:35, Levente Uzonyi wrote:
On Sun, 24 Apr 2011, Levente Uzonyi wrote:
snip
it didn't work for some reason. The execution always ran into the error 'computation has been terminated' in MethodContext >> #cannotReturn: and the method context (which stitches the continuation onto the stack) was broken (nil in temporaries where should be non nil values). Finally I tried it in SqueakVM instead of Cog and it worked. The tests are green, the example code
I tracked down this issue a bit more. The cause of the problem is that with CogVM the pc of thisContext is being set to nil when the execution leaves the context. This means, that [thisContext copy] will not be able to copy the pc of the context:
{ thisContext pc. thisContext copy pc } "==> #(23 nil)"
That would explain why I didn't come across it: I wrote the library on the SqueakVM.
Not exactly. Your serialization method works with Cog, because the problem occurs when #copy is sent to thisContext. Since your method is serializing the contexts and all their variables separately to a collection, therefore it works.
Levente
frank
On 2011/04/25 16:55, Levente Uzonyi wrote:
On Mon, 25 Apr 2011, Frank Shearar wrote:
On 2011/04/25 04:35, Levente Uzonyi wrote:
On Sun, 24 Apr 2011, Levente Uzonyi wrote:
snip
it didn't work for some reason. The execution always ran into the error 'computation has been terminated' in MethodContext >> #cannotReturn: and the method context (which stitches the continuation onto the stack) was broken (nil in temporaries where should be non nil values). Finally I tried it in SqueakVM instead of Cog and it worked. The tests are green, the example code
I tracked down this issue a bit more. The cause of the problem is that with CogVM the pc of thisContext is being set to nil when the execution leaves the context. This means, that [thisContext copy] will not be able to copy the pc of the context:
{ thisContext pc. thisContext copy pc } "==> #(23 nil)"
That would explain why I didn't come across it: I wrote the library on the SqueakVM.
Not exactly. Your serialization method works with Cog, because the problem occurs when #copy is sent to thisContext. Since your method is serializing the contexts and all their variables separately to a collection, therefore it works.
Oh, right. I didn't see that distinction because, on the SqueakVM, your refactoring worked!
frank
vm-dev@lists.squeakfoundation.org
-
Eliot Miranda -
Frank Shearar -
Levente Uzonyi