Raise params to the next frame when pushing #2145
Conversation
| @@ -13,46 +13,39 @@ class Scratch3ProcedureBlocks { | |||
| */ | |||
| getPrimitives () { | |||
| return { | |||
| procedures_definition: this.definition, | |||
There was a problem hiding this comment.
Maybe add a comment in place of this line for why procedures_definition isn't mentioned as a one of the 'primitives'
There was a problem hiding this comment.
On second thought, what if we made procedures_definition: null an entry here instead of removing it? In general, I don't like having undefined be something meaningful...
There was a problem hiding this comment.
Ah. Hmm. Setting this to null, we need to change a test in the runtime method that register's primitives. It tries to bind null like its a function. That is to say the register primitives algorithm only accepts functions currently.
I'd guess we'll need to change other things in there.
There was a problem hiding this comment.
| procedures_call: this.call, | ||
| argument_reporter_string_number: this.argumentReporterStringNumber, | ||
| argument_reporter_boolean: this.argumentReporterBoolean | ||
| }; | ||
| } | ||
|
|
||
| definition () { | ||
| // No-op: execute the blocks. |
There was a problem hiding this comment.
@cwillisf Do you have any concerns for extensionification of custom procs that the procedure definition function will not exist?
In general we expect either opcode or both opcode and func to be defined for a given block right? We should make sure we start to handle these non-existent functions well.
There was a problem hiding this comment.
We already handle non-existent functions (hats like event_whenflagclicked and shadows). Do we instead need to handle confirming that non-existence is expected?
| stackFrame.params = {}; | ||
| } | ||
| const stackFrame = this.stackFrame; | ||
| stackFrame.params = Object.create(null); |
There was a problem hiding this comment.
does it make sense to just use defaultParams here?
There was a problem hiding this comment.
Do we want to guard against overwriting already existing params here?
There was a problem hiding this comment.
Ooop. That is what I meant to do. 😄
There was a problem hiding this comment.
Hmm. Aw no, I think I misunderstood when I last responded.
This is compatible with Scratch 2 procedure behaviour to my knowledge. This PR swaps the order the stack is changed. In this PR the stack is pushed and then parameters are set. The parent procedure's stack frame is left alone and the new stack frame will start a new set of parameters.
There was a problem hiding this comment.
Ah okay.
That still leaves the question of whether we should use defaultParams here instead of explicitly using Object.create(null) again.
The engine supports blocks that do not have defined functions.
- Add pointer member to thread. This is the current executing block. - Add stackFrame member to thread. This is the current frame information like procedure parameters. This is a step potentially towards stack-less threads. With further modifications we could have stack and stackFrames be null if a script does not call a procedure.
|
I fixed the bug you found @kchadha. It was the dirty isLoop problem. I've added some tests to |
|
@mzgoddard just noticed what seems like another bug. I was testing project 304890904 from production locally w/the changes from this PR. Things seem to work great until I try to click on the original big red circle to delete it, and then it seems like the whole browser tab freezes. |
- Test that stopThisScript in a procedure stays in the procedure and moves it to the end where Sequencer can pop it normally
|
@kchadha Here's a minimal repro.
I found, tested, and fixed two bugs in The pre-existing bug is caused by a technical incompatibility with scratch 2. scratch-flash does one more thing in stopThisScript, it sets the block pointer to null scratch-flash/src/interpreter/Thread.as#L115. That would let Sequencer pop the procedure. Instead develop currently pops to the procedure call block, Sequencer leaves it alone and reruns the procedure call. Necessitating the executionContext check work around. There is a relevant unit test on stopThisScript but it looks like it depends on the technical incompatibility by looking at the stack and looking for the procedure call block id. It should instead check that stack is null. |
|
@mzgoddard I re-tested this PR after the bugfix you added, and I think it looks good to go. Could you add a code comment about the line you added in stopThisScript about why pushing null onto the stack is significant (and/or what bug it was fixing)? After that LGTM. |
|
|
Testing Plan: Description: This pr changes how Custom procedures store variables so they are more easily accessible to the vm while running the block. To test: Make a custom procedure that runs without screen refresh and repeat the above steps call the block recursively Make sure to add the variables from the custom procedure everywhere |
Resolves
Runtime performance related to stack operations including procedure parameters and util.stackFrame (thread.stackFrame.executionContext).
Proposed Changes
Related to #2147 and #2148.
Reason for Changes
The VM supports blocks without functions. An empty block function takes time to work through finding the function, calling it, and figuring out what to do with the result. So remove it and get on to other work.
The VM handles the stack correctly and will execute procedures_call twice. (I'm guessing this was a workaround for a bug we incidentally solved at some point.)
Always set to some object we can look up and set parameter values faster. Carry params forward when pushing the stack lets us immediately access the current procedure's parameters. We don't need to look at params in any other stack frames.
Since only Thread interacts with params we can use the benefits of a null prototype Object without the concerns of the same.
Resetting when we need to instead of always resetting can save us some time. Having executionContext already set lets us remove a branch statement when retrieving executionContext.
reportedmembers are used for promise returned blocks. As a thread is paused while the promise resolves the thread only needs to store one set ofreportedmembers. Moving that to the thread we can remove that from StackFrame and not worry about needing to unset it.Keep the "top" of the stack as direct members of the Thread. This way we can directly access them without needing to find out the length of the stack and get the last item.