Merge pull request #2126 from mzgoddard/runtime-script-cache-fix

Runtime script cache fix

Author: kchadha

src/engine/blocks-runtime-cache.js

@@ -0,0 +1,78 @@
+/**
+ * @fileoverview
+ * The BlocksRuntimeCache caches data about the top block of scripts so that
+ * Runtime can iterate a targeted opcode and iterate the returned set faster.
+ * Many top blocks need to match fields as well as opcode, since that matching
+ * compares strings in uppercase we can go ahead and uppercase the cached value
+ * so we don't need to in the future.
+ */
+
+/**
+ * A set of cached data about the top block of a script.
+ * @param {Blocks} container - Container holding the block and related data
+ * @param {string} blockId - Id for whose block data is cached in this instance
+ */
+class RuntimeScriptCache {
+    constructor (container, blockId) {
+        /**
+         * Container with block data for blockId.
+         * @type {Blocks}
+         */
+        this.container = container;
+
+        /**
+         * ID for block this instance caches.
+         * @type {string}
+         */
+        this.blockId = blockId;
+
+        const block = container.getBlock(blockId);
+        const fields = container.getFields(block);
+
+        /**
+         * Formatted fields or fields of input blocks ready for comparison in
+         * runtime.
+         *
+         * This is a clone of parts of the targeted blocks. Changes to these
+         * clones are limited to copies under RuntimeScriptCache and will not
+         * appear in the original blocks in their container. This copy is
+         * modified changing the case of strings to uppercase. These uppercase
+         * values will be compared later by the VM.
+         * @type {object}
+         */
+        this.fieldsOfInputs = Object.assign({}, fields);
+        if (Object.keys(fields).length === 0) {
+            const inputs = container.getInputs(block);
+            for (const input in inputs) {
+                if (!inputs.hasOwnProperty(input)) continue;
+                const id = inputs[input].block;
+                const inputBlock = container.getBlock(id);
+                const inputFields = container.getFields(inputBlock);
+                Object.assign(this.fieldsOfInputs, inputFields);
+            }
+        }
+        for (const key in this.fieldsOfInputs) {
+            const field = this.fieldsOfInputs[key] = Object.assign({}, this.fieldsOfInputs[key]);
+            if (field.value.toUpperCase) {
+                field.value = field.value.toUpperCase();
+            }
+        }
+    }
+}
+
+/**
+ * Get an array of scripts from a block container prefiltered to match opcode.
+ * @param {Blocks} container - Container of blocks
+ * @param {string} opcode - Opcode to filter top blocks by
+ */
+exports.getScripts = function () {
+    throw new Error('blocks.js has not initialized BlocksRuntimeCache');
+};
+
+/**
+ * Exposed RuntimeScriptCache class used by integration in blocks.js.
+ * @private
+ */
+exports._RuntimeScriptCache = RuntimeScriptCache;
+
+require('./blocks');

src/engine/blocks.js

@@ -5,6 +5,7 @@ const MonitorRecord = require('./monitor-record');
 const Clone = require('../util/clone');
 const {Map} = require('immutable');
 const BlocksExecuteCache = require('./blocks-execute-cache');
+const BlocksRuntimeCache = require('./blocks-runtime-cache');
 const log = require('../util/log');
 const Variable = require('./variable');
 const getMonitorIdForBlockWithArgs = require('../util/get-monitor-id');
@@ -74,7 +75,13 @@ class Blocks {
              * actively monitored.
              * @type {Array<{blockId: string, target: Target}>}
              */
-            _monitored: null
+            _monitored: null,
+
+            /**
+             * A cache of hat opcodes to collection of theads to execute.
+             * @type {object.<string, object>}
+             */
+            scripts: {}
         };
 
         /**
@@ -509,6 +516,7 @@ class Blocks {
         this._cache.procedureDefinitions = {};
         this._cache._executeCached = {};
         this._cache._monitored = null;
+        this._cache.scripts = {};
     }
 
     /**
@@ -1215,4 +1223,35 @@ BlocksExecuteCache.getCached = function (blocks, blockId, CacheType) {
     return cached;
 };
 
+/**
+ * Cache class constructor for runtime. Used to consider what threads should
+ * start based on hat data.
+ * @type {function}
+ */
+const RuntimeScriptCache = BlocksRuntimeCache._RuntimeScriptCache;
+
+/**
+ * Get an array of scripts from a block container prefiltered to match opcode.
+ * @param {Blocks} blocks - Container of blocks
+ * @param {string} opcode - Opcode to filter top blocks by
+ * @returns {Array.<RuntimeScriptCache>} - Array of RuntimeScriptCache cache
+ *   objects
+ */
+BlocksRuntimeCache.getScripts = function (blocks, opcode) {
+    let scripts = blocks._cache.scripts[opcode];
+    if (!scripts) {
+        scripts = blocks._cache.scripts[opcode] = [];
+
+        const allScripts = blocks._scripts;
+        for (let i = 0; i < allScripts.length; i++) {
+            const topBlockId = allScripts[i];
+            const block = blocks.getBlock(topBlockId);
+            if (block.opcode === opcode) {
+                scripts.push(new RuntimeScriptCache(blocks, topBlockId));
+            }
+        }
+    }
+    return scripts;
+};
+
 module.exports = Blocks;

src/engine/runtime.js

@@ -3,6 +3,7 @@ const {OrderedMap} = require('immutable');
 
 const ArgumentType = require('../extension-support/argument-type');
 const Blocks = require('./blocks');
+const BlocksRuntimeCache = require('./blocks-runtime-cache');
 const BlockType = require('../extension-support/block-type');
 const Profiler = require('./profiler');
 const Sequencer = require('./sequencer');
@@ -1434,16 +1435,11 @@ class Runtime extends EventEmitter {
      * @return {!Thread} The newly created thread.
      */
     _pushThread (id, target, opts) {
-        opts = Object.assign({
-            stackClick: false,
-            updateMonitor: false
-        }, opts);
-
         const thread = new Thread(id);
         thread.target = target;
-        thread.stackClick = opts.stackClick;
-        thread.updateMonitor = opts.updateMonitor;
-        thread.blockContainer = opts.updateMonitor ?
+        thread.stackClick = Boolean(opts && opts.stackClick);
+        thread.updateMonitor = Boolean(opts && opts.updateMonitor);
+        thread.blockContainer = thread.updateMonitor ?
             this.monitorBlocks :
             target.blocks;
 
@@ -1586,6 +1582,20 @@ class Runtime extends EventEmitter {
         }
     }
 
+    allScriptsByOpcodeDo (opcode, f, optTarget) {
+        let targets = this.executableTargets;
+        if (optTarget) {
+            targets = [optTarget];
+        }
+        for (let t = targets.length - 1; t >= 0; t--) {
+            const target = targets[t];
+            const scripts = BlocksRuntimeCache.getScripts(target.blocks, opcode);
+            for (let j = 0; j < scripts.length; j++) {
+                f(scripts[j], target);
+            }
+        }
+    }
+
     /**
      * Start all relevant hats.
      * @param {!string} requestedHatOpcode Opcode of hats to start.
@@ -1610,71 +1620,52 @@ class Runtime extends EventEmitter {
         }
 
         // Consider all scripts, looking for hats with opcode `requestedHatOpcode`.
-        this.allScriptsDo((topBlockId, target) => {
-            const blocks = target.blocks;
-            const block = blocks.getBlock(topBlockId);
-            const potentialHatOpcode = block.opcode;
-            if (potentialHatOpcode !== requestedHatOpcode) {
-                // Not the right hat.
-                return;
-            }
+        this.allScriptsByOpcodeDo(requestedHatOpcode, (script, target) => {
+            const {
+                blockId: topBlockId,
+                fieldsOfInputs: hatFields
+            } = script;
 
             // Match any requested fields.
             // For example: ensures that broadcasts match.
             // This needs to happen before the block is evaluated
             // (i.e., before the predicate can be run) because "broadcast and wait"
             // needs to have a precise collection of started threads.
-            let hatFields = blocks.getFields(block);
-
-            // If no fields are present, check inputs (horizontal blocks)
-            if (Object.keys(hatFields).length === 0) {
-                hatFields = {}; // don't overwrite the block's actual fields list
-                const hatInputs = blocks.getInputs(block);
-                for (const input in hatInputs) {
-                    if (!hatInputs.hasOwnProperty(input)) continue;
-                    const id = hatInputs[input].block;
-                    const inpBlock = blocks.getBlock(id);
-                    const fields = blocks.getFields(inpBlock);
-                    Object.assign(hatFields, fields);
-                }
-            }
-
-            if (optMatchFields) {
-                for (const matchField in optMatchFields) {
-                    if (hatFields[matchField].value.toUpperCase() !==
-                        optMatchFields[matchField]) {
-                        // Field mismatch.
-                        return;
-                    }
+            for (const matchField in optMatchFields) {
+                if (hatFields[matchField].value !== optMatchFields[matchField]) {
+                    // Field mismatch.
+                    return;
                 }
             }
 
             if (hatMeta.restartExistingThreads) {
                 // If `restartExistingThreads` is true, we should stop
                 // any existing threads starting with the top block.
-                for (let i = 0; i < instance.threads.length; i++) {
-                    if (instance.threads[i].topBlock === topBlockId &&
-                        !instance.threads[i].stackClick && // stack click threads and hat threads can coexist
-                        instance.threads[i].target === target) {
-                        newThreads.push(instance._restartThread(instance.threads[i]));
+                for (let i = 0; i < this.threads.length; i++) {
+                    if (this.threads[i].target === target &&
+                        this.threads[i].topBlock === topBlockId &&
+                        // stack click threads and hat threads can coexist
+                        !this.threads[i].stackClick) {
+                        newThreads.push(this._restartThread(this.threads[i]));
                         return;
                     }
                 }
             } else {
                 // If `restartExistingThreads` is false, we should
                 // give up if any threads with the top block are running.
-                for (let j = 0; j < instance.threads.length; j++) {
-                    if (instance.threads[j].topBlock === topBlockId &&
-                        instance.threads[j].target === target &&
-                        !instance.threads[j].stackClick && // stack click threads and hat threads can coexist
-                        instance.threads[j].status !== Thread.STATUS_DONE) {
+                for (let j = 0; j < this.threads.length; j++) {
+                    if (this.threads[j].target === target &&
+                        this.threads[j].topBlock === topBlockId &&
+                        // stack click threads and hat threads can coexist
+                        !this.threads[j].stackClick &&
+                        this.threads[j].status !== Thread.STATUS_DONE) {
                         // Some thread is already running.
                         return;
                     }
                 }
             }
             // Start the thread with this top block.
-            newThreads.push(instance._pushThread(topBlockId, target));
+            newThreads.push(this._pushThread(topBlockId, target));
         }, optTarget);
         // For compatibility with Scratch 2, edge triggered hats need to be processed before
         // threads are stepped. See ScratchRuntime.as for original implementation

src/engine/sequencer.js

@@ -177,6 +177,12 @@ class Sequencer {
         if (!currentBlockId) {
             // A "null block" - empty branch.
             thread.popStack();
+
+            // Did the null follow a hat block?
+            if (thread.stack.length === 0) {
+                thread.status = Thread.STATUS_DONE;
+                return;
+            }
         }
         // Save the current block ID to notice if we did control flow.
         while ((currentBlockId = thread.peekStack())) {

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

@@ -64,6 +64,52 @@ test('edge activated hat thread runs once every frame', t => {
     });
 });
 
+/**
+ * When a hat is added it should run in the next frame. Any block related
+ * caching should be reset.
+ */
+test('edge activated hat thread runs after being added to previously executed target', t => {
+    const vm = new VirtualMachine();
+    vm.attachStorage(makeTestStorage());
+
+    // Start VM, load project, and run
+    t.doesNotThrow(() => {
+        // Note: don't run vm.start(), we handle calling _step() manually in this test
+        vm.runtime.currentStepTime = Runtime.THREAD_STEP_INTERVAL;
+        vm.clear();
+        vm.setCompatibilityMode(false);
+        vm.setTurboMode(false);
+
+        vm.loadProject(project).then(() => {
+            t.equal(vm.runtime.threads.length, 0);
+
+            vm.runtime._step();
+            let threads = vm.runtime._lastStepDoneThreads;
+            t.equal(vm.runtime.threads.length, 0);
+            t.equal(threads.length, 1);
+            checkIsHatThread(t, vm, threads[0]);
+            t.assert(threads[0].status === Thread.STATUS_DONE);
+
+            // Add a second hat that should create a second thread
+            const hatBlock = threads[0].target.blocks.getBlock(threads[0].topBlock);
+            threads[0].target.blocks.createBlock(Object.assign(
+                {}, hatBlock, {id: 'hatblock2', next: null}
+            ));
+
+            // Check that the hat thread is added again when another step is taken
+            vm.runtime._step();
+            threads = vm.runtime._lastStepDoneThreads;
+            t.equal(vm.runtime.threads.length, 0);
+            t.equal(threads.length, 2);
+            checkIsHatThread(t, vm, threads[0]);
+            checkIsHatThread(t, vm, threads[1]);
+            t.assert(threads[0].status === Thread.STATUS_DONE);
+            t.assert(threads[1].status === Thread.STATUS_DONE);
+            t.end();
+        });
+    });
+});
+
 /**
  * If the hat doesn't finish evaluating within one frame, it shouldn't be added again
  * on the next frame. (We skip execution by setting the step time to 0)