Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread always done #2148

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mzgoddard
Copy link
Contributor

@mzgoddard mzgoddard commented Apr 25, 2019

Resolves

General performance around iterating threads during a step.

Proposed Changes

Depends on #2145 and #2147.
This PR is now standalone and does not depend on other PRs.

  • Remove Thread.isKilled
  • Test if a thread is STATUS_YIELD_TICK after STATUS_RUNNING
  • Remove stack.length === 0 tests

Reason for Changes

  • Remove Thread.isKilled

The current code base doesn't actually remove the thread so I'm not sure we need this additional data point.

Removing isKilled lets us remove the isKilled loop at the beginning of _step. Which currently, must loop over all threads.

The one way isKilled threads are configured that way is through stop thread, which also sets their status to DONE.

  • Test if a thread is STATUS_YIELD_TICK after STATUS_RUNNING

Generally most threads will be RUNNING and not YIELD_TICK. We can have the special case after the primary case in the same if/else branch chain and if it is YIELD_TICK, i-- to run it again and have the RUNNING branch activate. This removes a branch test from most thread execution.

  • Remove stack.length === 0 tests

The work in the depending PRs get us to a point where status === DONE when stack is length 0. So they should be synamous and we can use the one test.

@mzgoddard
Copy link
Contributor Author

mzgoddard commented May 8, 2019

@kchadha I refactored this PR so its now standalone.

This still achieves the goal that a thread with an empty stack array or stackFrame array has a status of DONE. This may leave the thread's peekStack at null and a status that is not DONE as long as the stack/stackFrames arrays are not length 0.

To achieve that it does a few added things that the prior dependee branches did.

  • Perform all sequencer and thread state logic in sequencer

Some logic in runtime.js and execute.js perform sequencer logic for hat and promise support. That logic is now either an existing thread method, a new sequencer method, or handled by Sequencer.stepThread.

  • add unwrapStack

Add a method to pop a thread's stack until it is exhausted, hits a loop or non-null block. The method's logic is reduced from its prior form to loop until the thread's status changes or peekStack is not null.

  • treat popping into a loop block literally as STATUS_YIELD

Part of unwrapStack replaces some warpMode consideration logic with setting the thread status to yield. Other code in stepThread can then consider if the yielded loop block should execute anyways due to warpMode or other cases.

  • remove continue; and return; statements from stepThread

Reorder the logic so cases using continue and return and be removed and allow the loop to execute normally and not exit prematurely.

Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments/changes requested, and it seems this branch needs to be updated against current develop to pull in changes from #2126

src/engine/sequencer.js Show resolved Hide resolved
src/engine/sequencer.js Outdated Show resolved Hide resolved
}
}
// Indicate the block that just executed.
thread.blockGlowInFrame = currentBlockId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this happen inside the while loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. blockGlowInFrame is one of those thread parts that I thought was odd to be in sequencer. I think it'd make the most sense being next to wherever requestScriptGlowInFrame is set.

Looking back at this, I've done just that. I moved blockGlowInFrame into execute alongside requestScript before the loops in execute(). That'll update the blockGlowInFrame every time we run a block.

src/engine/sequencer.js Outdated Show resolved Hide resolved
src/engine/sequencer.js Outdated Show resolved Hide resolved
Virtual machine only blocks allow us to ensure status === DONE when a
thread is DONE. So We do not need to test if the stack is length 0 or
if isKilled is set. Just make sure status === DONE.
@mzgoddard mzgoddard force-pushed the thread-always-done branch from 8768fa3 to 922bceb Compare June 3, 2019 16:31
@mzgoddard mzgoddard force-pushed the thread-always-done branch from 922bceb to bb25a58 Compare June 3, 2019 17:17
@kchadha
Copy link
Contributor

kchadha commented Jun 25, 2019

@mzgoddard, these changes are almost good to go, but I did notice that some stacks aren't glowing (they are in production). E.g. in this project 288115558, Sprite1 has a when [any] key pressed, next costume stack, and if I hold down a key (e.g. space), the stack will repeatedly glow in production but doesn't glow at all locally w/these changes pulled in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants