Remote block with tempVectors fails

[dionisiydk]

Transferring block with temp vectors logic does not work. And it can lead to some unexpected problems, infinite recursions or even image crash.
Simple example to reproduce is

| temp |
temp := 10.
remotePeer evaluate: [ temp := temp + 15  ].
temp.

Context should mark all tempVectors to be transferred by value.

[dionisiydk]

I created test for this in branch transferBlockWithTempVector

[LTangaF]

Switching from current master to the test branch plans to remove a lot of classes. I'm wondering what to put into the Metacello load to get closer to what your test branch originated from.

[LTangaF]

I suppose it probably just needs to be rebased. I'm not familiar with doing that in the context of an image, though. Do you have to re-issue the Metacello load after doing those sorts of git activities?

[dionisiydk]

It's because project was converted to Tonel. So all files were changed.
The branch only contains a test:

SeamlessRealCommunicationTests>>#testEvaluatingBlockWithTempManagedByVector
	| remotePeer result |
	remotePeer := self connectToServerPeer.
	"temp variable in following example is managed by temp vector 
	according to full block closure logic.
	Temp vectors should be forcibly transferred by value as integral part of context"
	self forkAndWait: [ | temp | 
		temp := 10.
		result := remotePeer evaluate: [ temp := temp + 5].
	]. 
	result should be: 15

[LTangaF]

I was thinking of your comment "Temp vectors should be forcibly transferred by value as integral part of context".
evaluate: doesn't have an evaluate:with: cousin to do value: calls with, so I'm guessing you mean we'd re-pack the block code and move the declaration and assignment of temp into the interior of the block, somehow determining that the block contains variables that are externally defined?

Through code re-packing, ending up with something that is instead effectively:

self forkAndWait: [
            result := remotePeer evaluate: [ | temp |
                                           temp := temp + 5 ] ]

[LTangaF]

So far, I've picked up the RBAssignmentNodes that are involved in the block interior using code like this:

| theblock temp assignmentNodes |
temp := 10.
theblock := [ temp := temp + 5 ].
"Get the temp variables that are actually referenced in the inner block"

assignmentNodes := theblock method ast body statements select: [ :thenode |
	thenode isAssignment and: [ (theblock asText findString: thenode variable name startingAt: 1) > 0 ] ].
assignmentNodes.

It's still a work in progress. I plan on injecting the tempVariables and assignmentNodes into the Block's compiled AST node's RBSequenceNode before sending it to remotePeer>>evaluate:

It's a work in progress, though, and I've got a question on the Development room of Discord about the the fact that the addNode messages don't seem to make any adjustments to the surrounding Node's assignment values, which are the pointers to their original locations in the original source.

[LTangaF]

It also just occurred to me, @dionisiydk, that what you meant by "forcibly transferred by value" could've been to establish BasysRemotePeer>>evaluate:with[:with[:with[:with]]] messages instead of the "defensive style" solution I'm working on.

I'm still new to SmallTalk programming, so it's possible that I've overthought this in that defensive direction. Probably my interpretation of "forcibly".

[dionisiydk]

Through code re-packing, ending up with something that is instead effectively:

self forkAndWait: [
            result := remotePeer evaluate: [ | temp |
                                           temp := temp + 5 ] ]

Semantically yet. It would lead to such equivalent code if we would pass temp vector by value.
It is required to fix a VM crash with more complex blocks. In current situation the remote side receives a corrupted block which state does not follow the VM expectation: the temp vector must be an Array instance but it is a proxy object.

From the other side the approach proposed here (pass by value) will break normal Smalltalk semantics: modification of the temp inside the block (on remote side) should be reflected on the home context (on client side) where the temp is declared and where it can be used after the return from the block.
A general solution should rewrite a block in a way where any remote temp writes would be encoded as explicit remote message (instead of simple store bytecode). My guess is that it is much harder to implement than what I proposed here.

I am not sure if you are aware what is a tempVector here. An example to understand:

| temp internalTempVector |
temp := #start.
internalTempVector := thisContext tempAt: 2.
[ temp := #end ].
internalTempVector  "==> #(#start)"

When temps written in the block are accessed from the outside of this block they are encoded with indirect arrays (a temp vector #(#start)).
Check this example without block:

| temp internalTempVector |
temp := #start.
internalTempVector := thisContext tempAt: 1.
internalTempVector "==> #start"

If we would know exactly indexes of tempVectors in the context the fix would be very trivial:

BlockClosure>>#prepareValueTransferBy: anObjectTransporter
        .........	
	outerContext tempVectors do: [:each |
             anObjectTransporter transferByValue: each] 
        ..........

[dionisiydk]

Also interesting that block rewrite would give us a solution for instance variables which is also broken now:

MyClass>>someDistrubutedMethod
        remotePeer evaluate: [myInstVar := #valueFromRemoteSide].

I think this task is super interesting to do but it could take a real time to implement.

[LTangaF]

I'm actually nearing completion on the block rewrite solution, but you threw me for a loop when you suggested that I may need to be able to reflect the value back into the original variable.

[LTangaF]

I guess my interpretation about this needing to be a defensive solution is correct, though.

[LTangaF]

Does the remote side have access to the local context (remote from it's perspective) ? We might be able to use communication via the context itself.

[dionisiydk]

I'm actually nearing completion on the block rewrite solution, but you threw me for a loop when you suggested that I may need to be able to reflect the value back into the original variable.

For some inspiration check how it works in SeamlessRemoteWorkspaceVariable. In RPlayground remote scripts actually updates the local variables value stored on the client side.

But it can be not very suitable here because we don't have a way to substitute the normal variables (the playground case is special anyway in local playground).

[LTangaF]

I'm still trying to get my head around temp vectors, actually.

I think your first explanation block was supposed to be:

| temp internalTempVector |
temp := #start.
internalTempVector := thisContext tempAt: 2.
[ temp := #end ] value.
internalTempVector

That actually yields "#(#end)", though. And I don't exactly get how the tempAt indexing actually works or the block closure's shorthand syntax for it.

[dionisiydk]

Does the remote side have access to the local context (remote from it's perspective) ? We might be able to use communication via the context itself.

On remote side outer contexts of the block are just copies of client side contexts. They are not proxies if you think about it (VM does not allow it).

Thinking about SeamlessRemoteWorkspaceVariable. Block and its home method can be rewritten (for being transferred) in the way that all variables are converted to such kind of special variable (SeamlessRemoteTemp, SeamlessRemoteInstVar). So any var access and var store would go through explicit message send to them where they will delegate an operation to the remote (client) side using one of reflective calls to get raw access to client side variable.

[dionisiydk]

I'm still trying to get my head around temp vectors, actually.

I think your first explanation block was supposed to be:

| temp internalTempVector |
temp := #start.
internalTempVector := thisContext tempAt: 2.
[ temp := #end ] value.
internalTempVector

Actually not. I intentionally did not put #value. Compiler does not care if block would be really executed or not. It sees the "closure logic" for variable access and compile it with array indirection.
And your #value case should prove my word: the actual temp value is really managed inside some array (tempVector).

That actually yields "#(#end)", though. And I don't exactly get how the tempAt indexing actually works or the block closure's shorthand syntax for it.

Me either. But I think it is not really important for example.

[LTangaF]

Well, I've already got the variable scanning piece to help establish which mappings I need to make to SeamlessRemoteInstVar, so I think I can probably solve it using that. Thank you for the pointers.

[MarcusDenker]

There is tempNamed:, too. Which reads the var from the context, taking temp vectors into account.

This is used to access temps in the doIt.

[MarcusDenker]

( I need to carefully read the thread, sadly no time today)

[LTangaF]

@dionisiydk, I've been digging at this for a couple of days. I think the issue is that outerContext needs a proxy back from remote to local, but I've been unable to figure out how to establish it.

[dionisiydk]

You can't have it as a proxy: the vm will crash with non local return from such block (I tried it long time ago but I am quite sure about it).

Did you check SeamlessRemoteWorkspaceVariable?
#emitStore:/#emitValue: compiles access as a message send to variable itself. It leads to remote send from the remote side to the client because variables are transferred as a proxy in method literals.
This should work for other variables. Client should convert given block/homeMethod pair to transferred version where all kind of variables are replaced and compiled with such indirection.

[LTangaF]

I looked at the class, but I just couldn't find good examples of how to use it. The one reference shows it being added as a binding in SeamlessRemoteScriptRenderer. Do I have to somehow have to get the binding for these registered in outerContext of the Block before it gets serialized?

[LTangaF]

Or is it just an in-place replacement of the compiled assignment nodes? Iterate through them and replace them like:

node value: (SeamlessRemoteWorkspaceVariable key: node name value: node value)

[dionisiydk]

Let's use an example. I am not sure that I understand correctly your thoughts.

The method:

| temp |
temp := 2 * 2.
^temp

should be converted to:

| temp |
``specialVarInLiteral write: (2 * 2).
^``specialVarInLiteral read

``specialVarInLiteral would be an instance of special kind of variable encoded as literal during transformation. Let's call it IndirectTempVariable. The implementation of write for example can be like:

IndirectTempVariable>>write: aValue
    tempDefiningContext tempAt: tempIndex put: aValue

Transformation code will create IndirectTempVariable instances with corresponding original context and tempIndex.
This example does not depend on Seamless and any remoting. It should be easy to test. And it can be even a part of Pharo AST package as interesting example of what we can do.

Then for Seamless scenario we will configure IndirectTempVariable to be transferred by reference. So on remote side #write: and #read: messages will be sent to client objects and therefore executed on original client context modifying original temp variables

[dionisiydk]

And the implementation of block transfer strategy would be like:

BlockClosure>>prepareValueTransferBy: anObjectTransporter
      self hasAssignments ifTrue: [
            ^anObjectTransporter transfer: self byReplacement: self transformToHaveIndirectAssignments]
     "original code"

Where #transformToHaveIndirectAssignments will create new transformed block with transformed home method where every assignment is replaced by appropriate indirect variable.

We will need to support other variables like IndirectInstanceVariable. But maybe existing infrastructure would allow to have only IndirectVariable configured with actualVariable instance (see OCAbstractVariable subclasses)

[LTangaF]

I was missing both hooks.

I was trying to figure out how to replace the RBLiteralValueNode in the AST block with that SeamlessRemoteWorkspaceVariable, which looks like it was specifically designed for use in SeamlessRemoteSyncContext only (it has methods used for binding and mapping). The RBLiteralValueNode is literally the only Variable node that actually does have a 'value' parameter, but it can only be assigned during instantiation and it seems to actually only work with literals.

So your suggestion about doing the replacement as a message node instead solve that issue, though it'd be slick to support the write and read behaviors in a more native way. At least this makes it very clear if you encounter it in the debugger later.

Thanks for these pointers. Obviously, you have more of the experience with the system to know all these tricks and I appreciate your guidance while I dig into things beyond mine.

[dionisiydk]

Now It should be slightly straightforward to implement. We unified all type of variables in Pharo 9 under single Variable hierarchy and they are directly used in AST:

• there is only RBVariableNode pointing to various kinds of Variable subclasses.
• we just need to implement "IndirectReflectiveVariable" to be used in transferred methods