Merge pull request #1683 from mzgoddard/sequencer-thread-cleanup

Sequencer thread cleanup

src/engine/runtime.js

@@ -241,6 +241,13 @@ class Runtime extends EventEmitter {
          */
         this._nonMonitorThreadCount = 0;
 
+        /**
+         * All threads that finished running and were removed from this.threads
+         * by behaviour in Sequencer.stepThreads.
+         * @type {Array<Thread>}
+         */
+        this._lastStepDoneThreads = null;
+
         /**
          * Currently known number of clones, used to enforce clone limit.
          * @type {number}
@@ -1663,6 +1670,9 @@ class Runtime extends EventEmitter {
         this._emitProjectRunStatus(
             this.threads.length + doneThreads.length -
                 this._getMonitorThreadCount([...this.threads, ...doneThreads]));
+        // Store threads that completed this iteration for testing and other
+        // internal purposes.
+        this._lastStepDoneThreads = doneThreads;
         if (this.renderer) {
             // @todo: Only render when this.redrawRequested or clones rendered.
             if (this.profiler !== null) {

src/engine/sequencer.js

@@ -74,7 +74,7 @@ class Sequencer {
         let numActiveThreads = Infinity;
         // Whether `stepThreads` has run through a full single tick.
         let ranFirstTick = false;
-        const doneThreads = this.runtime.threads.map(() => null);
+        const doneThreads = [];
         // Conditions for continuing to stepping threads:
         // 1. We must have threads in the list, and some must be active.
         // 2. Time elapsed must be less than WORK_TIME.
@@ -91,19 +91,17 @@ class Sequencer {
             }
 
             numActiveThreads = 0;
+            let stoppedThread = false;
             // Attempt to run each thread one time.
             for (let i = 0; i < this.runtime.threads.length; i++) {
                 const activeThread = this.runtime.threads[i];
+                // Check if the thread is done so it is not executed.
                 if (activeThread.stack.length === 0 ||
                     activeThread.status === Thread.STATUS_DONE) {
                     // Finished with this thread.
-                    doneThreads[i] = activeThread;
+                    stoppedThread = true;
                     continue;
                 }
-                // A thread was removed, added or this thread was restarted.
-                if (doneThreads[i] !== null) {
-                    doneThreads[i] = null;
-                }
                 if (activeThread.status === Thread.STATUS_YIELD_TICK &&
                     !ranFirstTick) {
                     // Clear single-tick yield from the last call of `stepThreads`.
@@ -130,6 +128,13 @@ class Sequencer {
                 if (activeThread.status === Thread.STATUS_RUNNING) {
                     numActiveThreads++;
                 }
+                // Check if the thread completed while it just stepped to make
+                // sure we remove it before the next iteration of all threads.
+                if (activeThread.stack.length === 0 ||
+                    activeThread.status === Thread.STATUS_DONE) {
+                    // Finished with this thread.
+                    stoppedThread = true;
+                }
             }
             // We successfully ticked once. Prevents running STATUS_YIELD_TICK
             // threads on the next tick.
@@ -138,28 +143,23 @@ class Sequencer {
             if (this.runtime.profiler !== null) {
                 this.runtime.profiler.stop();
             }
-        }
-        // Filter inactive threads from `this.runtime.threads`.
-        numActiveThreads = 0;
-        for (let i = 0; i < this.runtime.threads.length; i++) {
-            const thread = this.runtime.threads[i];
-            if (doneThreads[i] === null) {
-                this.runtime.threads[numActiveThreads] = thread;
-                numActiveThreads++;
-            }
-        }
-        this.runtime.threads.length = numActiveThreads;
 
-        // Filter undefined and null values from `doneThreads`.
+            // Filter inactive threads from `this.runtime.threads`.
-        let numDoneThreads = 0;
+            if (stoppedThread) {
-        for (let i = 0; i < doneThreads.length; i++) {
+                let nextActiveThread = 0;
-            const maybeThread = doneThreads[i];
+                for (let i = 0; i < this.runtime.threads.length; i++) {
-            if (maybeThread !== null) {
+                    const thread = this.runtime.threads[i];
-                doneThreads[numDoneThreads] = maybeThread;
+                    if (thread.stack.length !== 0 &&
-                numDoneThreads++;
+                        thread.status !== Thread.STATUS_DONE) {
+                        this.runtime.threads[nextActiveThread] = thread;
+                        nextActiveThread++;
+                    } else {
+                        doneThreads.push(thread);
+                    }
+                }
+                this.runtime.threads.length = nextActiveThread;
             }
         }
-        doneThreads.length = numDoneThreads;
 
         return doneThreads;
     }

test/integration/hat-threads-run-every-frame.js

@@ -45,15 +45,19 @@ test('edge activated hat thread runs once every frame', t => {
             t.equal(vm.runtime.threads.length, 0);
 
             vm.runtime._step();
-            t.equal(vm.runtime.threads.length, 1);
+            let threads = vm.runtime._lastStepDoneThreads;
-            checkIsHatThread(t, vm, vm.runtime.threads[0]);
+            t.equal(vm.runtime.threads.length, 0);
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.equal(threads.length, 1);
+            checkIsHatThread(t, vm, threads[0]);
+            t.assert(threads[0].status === Thread.STATUS_DONE);
 
             // Check that the hat thread is added again when another step is taken
             vm.runtime._step();
-            t.equal(vm.runtime.threads.length, 1);
+            threads = vm.runtime._lastStepDoneThreads;
-            checkIsHatThread(t, vm, vm.runtime.threads[0]);
+            t.equal(vm.runtime.threads.length, 0);
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.equal(threads.length, 1);
+            checkIsHatThread(t, vm, threads[0]);
+            t.assert(threads[0].status === Thread.STATUS_DONE);
             t.end();
         });
     });
@@ -79,15 +83,19 @@ test('edge activated hat thread not added twice', t => {
             t.equal(vm.runtime.threads.length, 0);
 
             vm.runtime._step();
+            let doneThreads = vm.runtime._lastStepDoneThreads;
             t.equal(vm.runtime.threads.length, 1);
+            t.equal(doneThreads.length, 0);
             const prevThread = vm.runtime.threads[0];
             checkIsHatThread(t, vm, vm.runtime.threads[0]);
             t.assert(vm.runtime.threads[0].status === Thread.STATUS_RUNNING);
 
             // Check that no new threads are added when another step is taken
             vm.runtime._step();
+            doneThreads = vm.runtime._lastStepDoneThreads;
             // There should now be one done hat thread and one new hat thread to run
             t.equal(vm.runtime.threads.length, 1);
+            t.equal(doneThreads.length, 0);
             checkIsHatThread(t, vm, vm.runtime.threads[0]);
             t.assert(vm.runtime.threads[0] === prevThread);
             t.end();
@@ -115,29 +123,33 @@ test('edge activated hat thread does not interrupt stack click thread', t => {
             t.equal(vm.runtime.threads.length, 0);
 
             vm.runtime._step();
-            t.equal(vm.runtime.threads.length, 1);
+            let doneThreads = vm.runtime._lastStepDoneThreads;
-            checkIsHatThread(t, vm, vm.runtime.threads[0]);
+            t.equal(vm.runtime.threads.length, 0);
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.equal(doneThreads.length, 1);
+            checkIsHatThread(t, vm, doneThreads[0]);
+            t.assert(doneThreads[0].status === Thread.STATUS_DONE);
 
             // Add stack click thread on this hat
-            vm.runtime.toggleScript(vm.runtime.threads[0].topBlock, {stackClick: true});
+            vm.runtime.toggleScript(doneThreads[0].topBlock, {stackClick: true});
 
             // Check that the hat thread is added again when another step is taken
             vm.runtime._step();
-            t.equal(vm.runtime.threads.length, 2);
+            doneThreads = vm.runtime._lastStepDoneThreads;
+            t.equal(vm.runtime.threads.length, 0);
+            t.equal(doneThreads.length, 2);
             let hatThread;
             let stackClickThread;
-            if (vm.runtime.threads[0].stackClick) {
+            if (doneThreads[0].stackClick) {
-                stackClickThread = vm.runtime.threads[0];
+                stackClickThread = doneThreads[0];
-                hatThread = vm.runtime.threads[1];
+                hatThread = doneThreads[1];
             } else {
-                stackClickThread = vm.runtime.threads[1];
+                stackClickThread = doneThreads[1];
-                hatThread = vm.runtime.threads[0];
+                hatThread = doneThreads[0];
             }
             checkIsHatThread(t, vm, hatThread);
             checkIsStackClickThread(t, vm, stackClickThread);
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.assert(doneThreads[0].status === Thread.STATUS_DONE);
-            t.assert(vm.runtime.threads[1].status === Thread.STATUS_DONE);
+            t.assert(doneThreads[1].status === Thread.STATUS_DONE);
             t.end();
         });
     });
@@ -163,7 +175,9 @@ test('edge activated hat thread does not interrupt stack click thread', t => {
             t.equal(vm.runtime.threads.length, 0);
 
             vm.runtime._step();
+            let doneThreads = vm.runtime._lastStepDoneThreads;
             t.equal(vm.runtime.threads.length, 1);
+            t.equal(doneThreads.length, 0);
             checkIsHatThread(t, vm, vm.runtime.threads[0]);
             t.assert(vm.runtime.threads[0].status === Thread.STATUS_RUNNING);
 
@@ -174,20 +188,22 @@ test('edge activated hat thread does not interrupt stack click thread', t => {
 
             // Check that the hat thread is added again when another step is taken
             vm.runtime._step();
-            t.equal(vm.runtime.threads.length, 2);
+            doneThreads = vm.runtime._lastStepDoneThreads;
+            t.equal(vm.runtime.threads.length, 0);
+            t.equal(doneThreads.length, 2);
             let hatThread;
             let stackClickThread;
-            if (vm.runtime.threads[0].stackClick) {
+            if (doneThreads[0].stackClick) {
-                stackClickThread = vm.runtime.threads[0];
+                stackClickThread = doneThreads[0];
-                hatThread = vm.runtime.threads[1];
+                hatThread = doneThreads[1];
             } else {
-                stackClickThread = vm.runtime.threads[1];
+                stackClickThread = doneThreads[1];
-                hatThread = vm.runtime.threads[0];
+                hatThread = doneThreads[0];
             }
             checkIsHatThread(t, vm, hatThread);
             checkIsStackClickThread(t, vm, stackClickThread);
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.assert(doneThreads[0].status === Thread.STATUS_DONE);
-            t.assert(vm.runtime.threads[1].status === Thread.STATUS_DONE);
+            t.assert(doneThreads[1].status === Thread.STATUS_DONE);
             t.end();
         });
     });

test/integration/monitor-threads-run-every-frame.js

@@ -55,13 +55,19 @@ test('monitor thread runs every frame', t => {
             t.equal(vm.runtime.threads.length, 0);
 
             vm.runtime._step();
-            checkMonitorThreadPresent(t, vm.runtime.threads);
+            let doneThreads = vm.runtime._lastStepDoneThreads;
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.equal(vm.runtime.threads.length, 0);
+            t.equal(doneThreads.length, 1);
+            checkMonitorThreadPresent(t, doneThreads);
+            t.assert(doneThreads[0].status === Thread.STATUS_DONE);
 
             // Check that both are added again when another step is taken
             vm.runtime._step();
-            checkMonitorThreadPresent(t, vm.runtime.threads);
+            doneThreads = vm.runtime._lastStepDoneThreads;
-            t.assert(vm.runtime.threads[0].status === Thread.STATUS_DONE);
+            t.equal(vm.runtime.threads.length, 0);
+            t.equal(doneThreads.length, 1);
+            checkMonitorThreadPresent(t, doneThreads);
+            t.assert(doneThreads[0].status === Thread.STATUS_DONE);
             t.end();
         });
     });
@@ -103,12 +109,18 @@ test('monitor thread not added twice', t => {
             t.equal(vm.runtime.threads.length, 0);
 
             vm.runtime._step();
+            let doneThreads = vm.runtime._lastStepDoneThreads;
+            t.equal(vm.runtime.threads.length, 1);
+            t.equal(doneThreads.length, 0);
             checkMonitorThreadPresent(t, vm.runtime.threads);
             t.assert(vm.runtime.threads[0].status === Thread.STATUS_RUNNING);
             const prevThread = vm.runtime.threads[0];
 
             // Check that both are added again when another step is taken
             vm.runtime._step();
+            doneThreads = vm.runtime._lastStepDoneThreads;
+            t.equal(vm.runtime.threads.length, 1);
+            t.equal(doneThreads.length, 0);
             checkMonitorThreadPresent(t, vm.runtime.threads);
             t.equal(vm.runtime.threads[0], prevThread);
             t.end();

test/integration/monitors.js

@@ -14,8 +14,10 @@ test('importing sb2 project with monitors', t => {
     // Evaluate playground data and exit
     vm.on('playgroundData', e => {
         const threads = JSON.parse(e.threads);
-        // All monitors should leave threads running
+        // All monitors should create threads that finish during the step and
-        t.equal(threads.length, 5);
+        // are revoved from runtime.threads.
+        t.equal(threads.length, 0);
+        t.equal(vm.runtime._lastStepDoneThreads.length, 5);
         // There should be one additional hidden monitor that is in the monitorState but
         // does not start a thread.
         t.equal(vm.runtime._monitorState.size, 6);

test/unit/engine_sequencer.js

@@ -181,8 +181,7 @@ test('stepThreads', t => {
     t.strictEquals(s.stepThreads().length, 0);
     generateThread(r);
     t.strictEquals(r.threads.length, 1);
-    t.strictEquals(s.stepThreads().length, 0);
+    // Threads should be marked DONE and removed in the same step they finish.
-    r.threads[0].status = Thread.STATUS_RUNNING;
     t.strictEquals(s.stepThreads().length, 1);
     
     t.end();