Broadcast and wait should not yield when dirty until return?

[griffpatch]

Here is a simplified project demonstrating a feature that I use in most of my scratch projects that acts differently in Scratch 2 & 3.

https://llk.github.io/scratch-gui/develop/#184390187

In Scratch 2 you can execute blocks that update the screen and then issue a broadcast and wait without the Scratch runtime yielding to do a screen update until the broadcast and wait completes.

In Scratch 3 the scratch vm apparently stops to update the screen before the broadcast and wait is issued? This results in projects running at half speed in Scratch 3 even when they are doing very little.

Another interesting issue... Try the project on turbo mode. It then runs at 30fps exactly... But remove the broadcast and wait and it jumps to run at 2730 fps. Now in Scratch 2 it runs super fast with the broadcast and wait as well as without. So something about broadcast and waits appear to be holding up turbo mode too?

More Info

I have identified that it is not to do with loops. See: http://llk.github.io/scratch-vm/#184430433 In this project, if it runs at 30fps, then both cats should 'just' pass the line.

Turbo Off, 60fps mode -> Runs at 30fps Turbo Off, 30fps mode -> Runs at 15fps Turbo On, 60fps mode -> Runs at 60fps Turbo On, 30fps mode -> Runs at 30fps

So the questions are, why does a broadcast and wait cause the turbo mode to yield? And why do we get two yields instead of one when running without turbo.

Steps to Reproduce

Try this project in Scratch 2 and Scratch 3.

https://llk.github.io/scratch-gui/develop/#184390187

Operating System and Browser

Windows 10 Chrome

[thisandagain]

@mzgoddard This might be a good / low hanging issue to prioritize.

[paulkaplan]

I have a slightly simplified repro of the "broadcast and wait always yielding": https://llk.github.io/scratch-gui/develop/#184450398

This project runs very slowly, even though in 2.0 it runs at full speed.

[paulkaplan]

I believe this may be related to this issue? https://github.com/LLK/scratch-vm/issues/688

[griffpatch]

I'm not sure... although I did wonder about that... but do broadcast and waits actual use promises? I couldn't find anywhere obvious that linked into the promises from the broadcast and wait code...

[griffpatch]

Ok, so I have a fix for this, although I'm not sure if there are any ramifications.

See comit: 1b1bfed125745d5fc20db98ff8ee5f866d8d67bc

When a broadcast and wait executes, it ends up in an unending loop doing the same checks over and over for 1/60th of a second because the thread is active, but the thread.stack is an empty list.

If you add the empty stack check to the isActiveThreads then all the issues go away. I was quite amazed at the improvement as I was actually able to play turrican 2 in scratch 3 at 60fps!! Too fast!!. (https://griffpatch.github.io/scratch-vm/#174671513 - note, once green flag clicked, you need to click on the Scratch VM Playground title to allow keyboard control.)

isActiveThread (thread) {
    return thread.stack.length > 0 && this.threads.indexOf(thread) > -1;
}

I think this in combination with your recent optimisations has made things unbelievably better! Well done guys, this is astonishing ;)

Griffpatch

[paulkaplan]

Oh wow cool @griffpatch. I do wonder if that check should go inside of isActiveThread or if it should just be another condition in that some function

[griffpatch]

Yes, I don't know either, please do have a rummage and see if you can fathom it... I don't have a lot of time to look deeper, but after debugging a while this was my first idea to try that might work...

On 6 Nov 2017 22:02, "Paul Kaplan" notifications@github.com wrote:

Oh wow cool @griffpatch https://github.com/griffpatch. I do wonder if that check should go inside of isActiveThread or if it should just be another condition in that some function

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LLK/scratch-vm/issues/760#issuecomment-342302515, or mute the thread https://github.com/notifications/unsubscribe-auth/AGbNvj7PsJpumm2Zc2XyP7cfLrWkC6IKks5sz4HfgaJpZM4QS62R .

[mzgoddard]

@paulkaplan @griffpatch broadcastAndWait and switchBackdropAndWait are the only parts of the vm that use isActiveThread. The current test checks if the thread is in the list of current runtime.threads. runtime.threads is not updated until the end of Sequencer.stepThreads after which scratch is going to do a render update and wait for the next frame.

Seems like there are three solutions:

1. Filter out done threads (execute considers threads with a stack length of 0 or with a status of DONE to be non-active) after each outer loop in Sequencer.stepThreads.
2. Add a new method for blocks to call to test if a thread is complete the same way as stepThreads.
3. Do option 2 in isActiveThreads.

I think the best option would be to test if a thread is active the same way Sequencer.stepThreads does (option 3).

[mzgoddard]

Relevant stepThreads code:

https://github.com/LLK/scratch-vm/blob/b0870518a4ffe7cf991159ea1101d4d4c6a26ba8/src/engine/sequencer.js#L54-L55

[mzgoddard]

Relevant isActiveThread code:

https://github.com/LLK/scratch-vm/blob/b0870518a4ffe7cf991159ea1101d4d4c6a26ba8/src/engine/runtime.js#L686-L688

[griffpatch]

Turns out my fix is not good. It breaks the broadcast and wait when the receiving hat block takes more than one cycle to execute:

This project should cycle back and forward and not move across the screen... but it does now! oops...

https://griffpatch.github.io/scratch-vm/#184656394

What is happening is that the broadcast and wait is simply 'not' waiting, such that a subsequent broadcast is executing and restarting the receiving hat block before it gets to the 'move -2' block.

[griffpatch]

@mzgoddard I think you are spot on there, as it turns out though I think 2 & 3 do not work as expected... It seems that the broadcast and wait thinks that no threads are started until after the initial loop through the threads is complete perhaps? so your option one of post filtering after each loop works very well, whereas my initial fix of checking for empty thread stacks is floored. I moved the filtering of threads up inside the stepping loop and this fixed both the waiting issue and the not waiting long enough issue.

Am I right in saying you (or someone else) were looking at improving the efficiency of that filtering loop too? I'm not too keen on adding such a loop as it stands inside the stepping critical loop?

https://github.com/griffpatch/scratch-vm/commit/e0515c58ad3bb3cd472a35a0c696c1ba57fa9cb7?diff=unified#diff-73faef1f944dbb62b51218068b0669e3

@paulkaplan @griffpatch broadcastAndWait and switchBackdropAndWait are the only parts of the vm that use isActiveThread. The current test checks if the thread is in the list of current runtime.threads. runtime.threads is not updated until the end of Sequencer.stepThreads after which scratch is going to do a render update and wait for the next frame.

Seems like there are three solutions:

1. Filter out done threads (execute considers threads with a stack length of 0 or with a status of DONE to be non-active) after each outer loop in Sequencer.stepThreads.
2. Add a new method for blocks to call to test if a thread is complete the same way as stepThreads.
3. Do option 2 in isActiveThreads. I think the best option would be to test if a thread is active the same way Sequencer.stepThreads does (option 3).

[griffpatch]

With regard to improving performance of the doneThreads... can you see any issue with this refactor: 90f82c3611af05f01ee134d6e83897c27d11cbae ?

It appears to run much faster with the same result as far as my analysis can go right now?

Griffpatch

[mzgoddard]

@griffpatch https://github.com/LLK/scratch-vm/pull/758 was the filtering change merged in just a bit ago today. Its performance improvement changes the vm's behaviour as little as possible. I'm not sure yet what side-effects filtering earlier may have.

Theory wise, splice may well have similar issue compared with the prior indexOf implementation. Inside that loop they can take exponential time to execute. splice would cost less than indexOf but most likely not less than the change that is now in from #758. splice removes the specified number of members and then goes through the rest of the array shifting non-removed values to lower indices. If the done test is cheap (stack === 0 or status === DONE) just iterating though the same list could well be cheaper with how good JS vms are at iterating through Arrays.

But that's all theory best to actually compare the two implementations with actual projects. Projects with high thread turnover like https://scratch.mit.edu/projects/14844969/ are a good place to test filtering changes performance wise.

[mzgoddard]

@griffpatch I think this is actually two bugs with one symptom. Your original change in isActiveThread is a fix for one bug. And the second bug is Runtime.startHats. startHats doesn't add restarted threads to the list of new threads that is returned to and relied on by broadcastAndWait. That lets broadcastAndWait think no threads were started though they are in the runtime.threads list. With that second bug it quickly runs through any broadcastAndWait calls with the same message until the vm actively removes the thread from the list.

[kchadha]

@mzgoddard This issue was referenced in #768, is it fixed or is there more to do?

[mzgoddard]

@kchadha I don't believe anything else is needed for this. #768 should be good.

[towerofnix]

BTW, looks like this is still an issue?

[thisandagain]

@mzgoddard @ktbee @kchadha Any thoughts on this?

[kchadha]

I think this issue is re-fixed with the changes in #1683. Testing the changes in that PR locally, the project that @towerofnix posted seems to be performing the same as in 2.0.

[griffpatch]

Another test project you can use to test this project with a FPS counter: https://llk.github.io/scratch-gui/develop/#258800864