From 9645a513b1bbdf365e3b86fda136a31db67b0fb0 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Fri, 19 May 2017 12:28:05 -0400 Subject: [PATCH 1/9] Only send monitor update on change --- src/engine/runtime.js | 48 ++++++++++++++++++++++++++++++------- test/unit/engine_runtime.js | 46 +++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 92f98e434..bae3d0bae 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -2,6 +2,7 @@ const EventEmitter = require('events'); const Sequencer = require('./sequencer'); const Blocks = require('./blocks'); const Thread = require('./thread'); +const {Map} = require('immutable'); // Virtual I/O devices. const Clock = require('../io/clock'); @@ -114,7 +115,12 @@ class Runtime extends EventEmitter { /** * List of all monitors. */ - this._monitorState = {}; + this._monitorState = Map({}); + + /** + * Monitor state from last tick + */ + this._prevMonitorState = Map({}); /** * Whether the project is in "turbo mode." @@ -699,10 +705,10 @@ class Runtime extends EventEmitter { this._refreshTargets = false; } - // @todo(vm#570) only emit if monitors has changed since last time. - this.emit(Runtime.MONITORS_UPDATE, - Object.keys(this._monitorState).map(key => this._monitorState[key]) - ); + if (!this._prevMonitorState.equals(this._monitorState)) { + this.emit(Runtime.MONITORS_UPDATE, this._monitorState.toArray()); + } + this._prevMonitorState = this._monitorState; } /** @@ -877,7 +883,7 @@ class Runtime extends EventEmitter { * @param {!object} monitor Monitor to add. */ requestAddMonitor (monitor) { - this._monitorState[monitor.id] = monitor; + this._monitorState = this._monitorState.set(monitor.id, monitor); } /** @@ -886,18 +892,42 @@ class Runtime extends EventEmitter { * @param {!object} monitor Monitor to update. */ requestUpdateMonitor (monitor) { - if (this._monitorState.hasOwnProperty(monitor.id)) { - this._monitorState[monitor.id] = Object.assign({}, this._monitorState[monitor.id], monitor); + if (this._monitorState.has(monitor.id) && + this._monitorWouldChange(this._monitorState.get(monitor.id), monitor)) { + this._monitorState = + this._monitorState.set(monitor.id, Object.assign({}, this._monitorState.get(monitor.id), monitor)); } } + /** + * Whether or not the monitor would change when the new state is applied. New state has its properties + * applied to the current state, it does not completely overwrite the current state. For this to return + * accurately, the changes being applied should be on primitive types, otherwise it may false-positive. + * @param {!object} currentMonitorState the state of a monitor, the values in this._monitorState + * @param {!object} newMonitorDelta the state to be applied to a monitor. The IDs of current and new monitor + * should match. + * @return {boolean} whether the new state, when applied to the current state, would change the current state. + */ + _monitorWouldChange (currentMonitorState, newMonitorDelta) { + for (const prop in newMonitorDelta) { + if (currentMonitorState.hasOwnProperty(prop)) { + // Strict equals to check if properties change; switch this to deep equals if + // we expect monitors to have changing state that is not primative. + if (currentMonitorState[prop] !== newMonitorDelta[prop]) return true; + } else { + return true; // The new state would add a new property to the monitor + } + } + return false; + } + /** * Removes a monitor from the state. Does nothing if the monitor already does * not exist in the state. * @param {!object} monitorId ID of the monitor to remove. */ requestRemoveMonitor (monitorId) { - delete this._monitorState[monitorId]; + this._monitorState = this._monitorState.delete(monitorId); } /** diff --git a/test/unit/engine_runtime.js b/test/unit/engine_runtime.js index be431186d..6fd04eefd 100644 --- a/test/unit/engine_runtime.js +++ b/test/unit/engine_runtime.js @@ -10,3 +10,49 @@ test('spec', t => { t.end(); }); + +test('monitorWouldChange_false', t => { + const r = new Runtime(); + const currentMonitorState = { + id: 'xklj4#!', + category: 'data', + label: 'turtle whereabouts', + value: '25', + x: 0, + y: 0 + }; + const newMonitorDelta = { + id: 'xklj4#!', + value: String(25) + }; + t.equals(false, r._monitorWouldChange(currentMonitorState, newMonitorDelta)); + t.end(); +}); + +test('monitorWouldChange_true', t => { + const r = new Runtime(); + const currentMonitorState = { + id: 'xklj4#!', + category: 'data', + label: 'turtle whereabouts', + value: '25', + x: 0, + y: 0 + }; + + // Value change + let newMonitorDelta = { + id: 'xklj4#!', + value: String(24) + }; + t.equal(true, r._monitorWouldChange(currentMonitorState, newMonitorDelta)); + + // Prop change + newMonitorDelta = { + id: 'xklj4#!', + moose: 7 + }; + t.equal(true, r._monitorWouldChange(currentMonitorState, newMonitorDelta)); + + t.end(); +}); From 66c8d9550dfcc6c76ca790128940c93ded3307c7 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Fri, 19 May 2017 12:42:37 -0400 Subject: [PATCH 2/9] add dep to package.json --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 0308aa6f7..44bb99a91 100644 --- a/package.json +++ b/package.json @@ -36,6 +36,7 @@ "gh-pages": "^0.12.0", "highlightjs": "^9.8.0", "htmlparser2": "3.9.2", + "immutable": "3.8.1", "json": "^9.0.4", "lodash.defaultsdeep": "4.6.0", "minilog": "3.1.0", From 48c51ab2ecc248d99632e0908ee84572c895cb7a Mon Sep 17 00:00:00 2001 From: DD Liu Date: Fri, 19 May 2017 17:28:00 -0400 Subject: [PATCH 3/9] switch to nested immutables state --- src/engine/blocks.js | 30 ++++++++++----------- src/engine/execute.js | 5 ++-- src/engine/runtime.js | 38 ++++++-------------------- test/unit/engine_runtime.js | 53 ++++++++++++++++++++++++------------- 4 files changed, 59 insertions(+), 67 deletions(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index ebe113f89..b26f6f529 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -1,6 +1,7 @@ const adapter = require('./adapter'); const mutationAdapter = require('./mutation-adapter'); const xmlEscape = require('../util/xml-escape'); +const {Map} = require('immutable'); /** * @fileoverview @@ -293,22 +294,19 @@ class Blocks { if (optRuntime && wasMonitored && !block.isMonitored) { optRuntime.requestRemoveMonitor(block.id); } else if (optRuntime && !wasMonitored && block.isMonitored) { - optRuntime.requestAddMonitor( - // Ensure that value is not undefined, since React requires it - { - // @todo(vm#564) this will collide if multiple sprites use same block - id: block.id, - category: 'data', - // @todo(vm#565) how to handle translation here? - label: block.opcode, - // @todo(vm#565) for numerical values with decimals, some countries use comma - value: '', - x: 0, - // @todo(vm#566) Don't require sending x and y when instantiating a - // monitor. If it's not preset the GUI should decide. - y: 0 - } - ); + optRuntime.requestAddMonitor(Map({ + // @todo(vm#564) this will collide if multiple sprites use same block + id: block.id, + category: 'data', + // @todo(vm#565) how to handle translation here? + label: block.opcode, + // @todo(vm#565) for numerical values with decimals, some countries use comma + value: '', // Ensure that value is not undefined, since React requires it + x: 0, + // @todo(vm#566) Don't require sending x and y when instantiating a + // monitor. If it's not preset the GUI should decide. + y: 0 + })); } break; } diff --git a/src/engine/execute.js b/src/engine/execute.js index 34e2e91be..3dfa7c1a9 100644 --- a/src/engine/execute.js +++ b/src/engine/execute.js @@ -1,5 +1,6 @@ const log = require('../util/log'); const Thread = require('./thread'); +const {Map} = require('immutable'); /** * Utility function to determine if a value is a Promise. @@ -97,10 +98,10 @@ const execute = function (sequencer, thread) { runtime.visualReport(currentBlockId, resolvedValue); } if (thread.updateMonitor) { - runtime.requestUpdateMonitor({ + runtime.requestUpdateMonitor(Map({ id: currentBlockId, value: String(resolvedValue) - }); + })); } } // Finished any yields. diff --git a/src/engine/runtime.js b/src/engine/runtime.js index bae3d0bae..bdc6c29a4 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -706,7 +706,8 @@ class Runtime extends EventEmitter { } if (!this._prevMonitorState.equals(this._monitorState)) { - this.emit(Runtime.MONITORS_UPDATE, this._monitorState.toArray()); + const monitorStateObj = this._monitorState.toJS(); + this.emit(Runtime.MONITORS_UPDATE, Object.keys(monitorStateObj).map(key => monitorStateObj[key])); } this._prevMonitorState = this._monitorState; } @@ -880,51 +881,28 @@ class Runtime extends EventEmitter { /** * Add a monitor to the state. If the monitor already exists in the state, * overwrites it. - * @param {!object} monitor Monitor to add. + * @param {!Map} monitor Monitor to add. */ requestAddMonitor (monitor) { - this._monitorState = this._monitorState.set(monitor.id, monitor); + this._monitorState = this._monitorState.set(monitor.get('id'), monitor); } /** * Update a monitor in the state. Does nothing if the monitor does not already * exist in the state. - * @param {!object} monitor Monitor to update. + * @param {!Map} monitor Monitor to update. */ requestUpdateMonitor (monitor) { - if (this._monitorState.has(monitor.id) && - this._monitorWouldChange(this._monitorState.get(monitor.id), monitor)) { + if (this._monitorState.has(monitor.get('id'))) { this._monitorState = - this._monitorState.set(monitor.id, Object.assign({}, this._monitorState.get(monitor.id), monitor)); + this._monitorState.set(monitor.get('id'), this._monitorState.get(monitor.get('id')).merge(monitor)); } } - /** - * Whether or not the monitor would change when the new state is applied. New state has its properties - * applied to the current state, it does not completely overwrite the current state. For this to return - * accurately, the changes being applied should be on primitive types, otherwise it may false-positive. - * @param {!object} currentMonitorState the state of a monitor, the values in this._monitorState - * @param {!object} newMonitorDelta the state to be applied to a monitor. The IDs of current and new monitor - * should match. - * @return {boolean} whether the new state, when applied to the current state, would change the current state. - */ - _monitorWouldChange (currentMonitorState, newMonitorDelta) { - for (const prop in newMonitorDelta) { - if (currentMonitorState.hasOwnProperty(prop)) { - // Strict equals to check if properties change; switch this to deep equals if - // we expect monitors to have changing state that is not primative. - if (currentMonitorState[prop] !== newMonitorDelta[prop]) return true; - } else { - return true; // The new state would add a new property to the monitor - } - } - return false; - } - /** * Removes a monitor from the state. Does nothing if the monitor already does * not exist in the state. - * @param {!object} monitorId ID of the monitor to remove. + * @param {!string} monitorId ID of the monitor to remove. */ requestRemoveMonitor (monitorId) { this._monitorState = this._monitorState.delete(monitorId); diff --git a/test/unit/engine_runtime.js b/test/unit/engine_runtime.js index 6fd04eefd..dd60c5eda 100644 --- a/test/unit/engine_runtime.js +++ b/test/unit/engine_runtime.js @@ -1,5 +1,6 @@ const test = require('tap').test; const Runtime = require('../../src/engine/runtime'); +const {Map} = require('immutable'); test('spec', t => { const r = new Runtime(); @@ -11,48 +12,62 @@ test('spec', t => { t.end(); }); -test('monitorWouldChange_false', t => { +test('monitorStateEquals', t => { const r = new Runtime(); - const currentMonitorState = { - id: 'xklj4#!', + const id = 'xklj4#!'; + const prevMonitorState = Map({ + id, category: 'data', label: 'turtle whereabouts', value: '25', x: 0, y: 0 - }; - const newMonitorDelta = { - id: 'xklj4#!', + }); + const newMonitorDelta = Map({ + id, value: String(25) - }; - t.equals(false, r._monitorWouldChange(currentMonitorState, newMonitorDelta)); + }); + r.requestAddMonitor(prevMonitorState); + r.requestUpdateMonitor(newMonitorDelta); + + t.equals(true, prevMonitorState.equals(r._monitorState.get(id))); + t.equals(String(25), r._monitorState.get(id).get('value')); t.end(); }); -test('monitorWouldChange_true', t => { +test('monitorStateDoesNotEqual', t => { const r = new Runtime(); - const currentMonitorState = { - id: 'xklj4#!', + const id = 'xklj4#!'; + const prevMonitorState = Map({ + id, category: 'data', label: 'turtle whereabouts', value: '25', x: 0, y: 0 - }; + }); // Value change - let newMonitorDelta = { - id: 'xklj4#!', + let newMonitorDelta = Map({ + id, value: String(24) - }; - t.equal(true, r._monitorWouldChange(currentMonitorState, newMonitorDelta)); + }); + r.requestAddMonitor(prevMonitorState); + r.requestUpdateMonitor(newMonitorDelta); + + t.equals(false, prevMonitorState.equals(r._monitorState.get(id))); + t.equals(String(24), r._monitorState.get(id).get('value')); // Prop change - newMonitorDelta = { + newMonitorDelta = Map({ id: 'xklj4#!', moose: 7 - }; - t.equal(true, r._monitorWouldChange(currentMonitorState, newMonitorDelta)); + }); + r.requestUpdateMonitor(newMonitorDelta); + + t.equals(false, prevMonitorState.equals(r._monitorState.get(id))); + t.equals(String(24), r._monitorState.get(id).get('value')); + t.equals(7, r._monitorState.get(id).get('moose')); t.end(); }); From acdc783f27aa3e7fda756a5e04d1c4758cf61129 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Mon, 22 May 2017 17:31:53 -0400 Subject: [PATCH 4/9] emit immutable --- src/engine/runtime.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index bdc6c29a4..8ecbb6b1f 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -706,10 +706,9 @@ class Runtime extends EventEmitter { } if (!this._prevMonitorState.equals(this._monitorState)) { - const monitorStateObj = this._monitorState.toJS(); - this.emit(Runtime.MONITORS_UPDATE, Object.keys(monitorStateObj).map(key => monitorStateObj[key])); + this.emit(Runtime.MONITORS_UPDATE, this._monitorState); + this._prevMonitorState = this._monitorState; } - this._prevMonitorState = this._monitorState; } /** From 1902848ea4e67683bc4371ede0e54c6e223041ac Mon Sep 17 00:00:00 2001 From: DD Liu Date: Wed, 24 May 2017 15:42:29 -0400 Subject: [PATCH 5/9] Switch to ordered maps of monitor records for monitor state --- src/engine/blocks.js | 4 ++-- src/engine/runtime.js | 14 ++++++++------ test/unit/engine_runtime.js | 13 ++++++------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index b26f6f529..19921f832 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -1,7 +1,7 @@ const adapter = require('./adapter'); const mutationAdapter = require('./mutation-adapter'); const xmlEscape = require('../util/xml-escape'); -const {Map} = require('immutable'); +const MonitorRecord = require('./records'); /** * @fileoverview @@ -294,7 +294,7 @@ class Blocks { if (optRuntime && wasMonitored && !block.isMonitored) { optRuntime.requestRemoveMonitor(block.id); } else if (optRuntime && !wasMonitored && block.isMonitored) { - optRuntime.requestAddMonitor(Map({ + optRuntime.requestAddMonitor(MonitorRecord({ // @todo(vm#564) this will collide if multiple sprites use same block id: block.id, category: 'data', diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 8ecbb6b1f..26dc4d354 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -2,7 +2,7 @@ const EventEmitter = require('events'); const Sequencer = require('./sequencer'); const Blocks = require('./blocks'); const Thread = require('./thread'); -const {Map} = require('immutable'); +const {OrderedMap} = require('immutable'); // Virtual I/O devices. const Clock = require('../io/clock'); @@ -113,14 +113,14 @@ class Runtime extends EventEmitter { this._refreshTargets = false; /** - * List of all monitors. + * Ordered map of all monitors, which are MonitorReporter objects. */ - this._monitorState = Map({}); + this._monitorState = OrderedMap({}); /** * Monitor state from last tick */ - this._prevMonitorState = Map({}); + this._prevMonitorState = OrderedMap({}); /** * Whether the project is in "turbo mode." @@ -880,7 +880,7 @@ class Runtime extends EventEmitter { /** * Add a monitor to the state. If the monitor already exists in the state, * overwrites it. - * @param {!Map} monitor Monitor to add. + * @param {!MonitorRecord} monitor Monitor to add. */ requestAddMonitor (monitor) { this._monitorState = this._monitorState.set(monitor.get('id'), monitor); @@ -889,7 +889,9 @@ class Runtime extends EventEmitter { /** * Update a monitor in the state. Does nothing if the monitor does not already * exist in the state. - * @param {!Map} monitor Monitor to update. + * @param {!Map} monitor Monitor values to update. Values on the monitor with overwrite + * values on the old monitor with the same ID. If a value isn't defined on the new monitor, + * the old monitor will keep its old value. */ requestUpdateMonitor (monitor) { if (this._monitorState.has(monitor.get('id'))) { diff --git a/test/unit/engine_runtime.js b/test/unit/engine_runtime.js index dd60c5eda..8087bab63 100644 --- a/test/unit/engine_runtime.js +++ b/test/unit/engine_runtime.js @@ -1,5 +1,6 @@ const test = require('tap').test; const Runtime = require('../../src/engine/runtime'); +const MonitorRecord = require('../../src/engine/records'); const {Map} = require('immutable'); test('spec', t => { @@ -15,7 +16,7 @@ test('spec', t => { test('monitorStateEquals', t => { const r = new Runtime(); const id = 'xklj4#!'; - const prevMonitorState = Map({ + const prevMonitorState = MonitorRecord({ id, category: 'data', label: 'turtle whereabouts', @@ -38,13 +39,11 @@ test('monitorStateEquals', t => { test('monitorStateDoesNotEqual', t => { const r = new Runtime(); const id = 'xklj4#!'; - const prevMonitorState = Map({ + const prevMonitorState = MonitorRecord({ id, category: 'data', label: 'turtle whereabouts', - value: '25', - x: 0, - y: 0 + value: '25' }); // Value change @@ -61,13 +60,13 @@ test('monitorStateDoesNotEqual', t => { // Prop change newMonitorDelta = Map({ id: 'xklj4#!', - moose: 7 + x: 7 }); r.requestUpdateMonitor(newMonitorDelta); t.equals(false, prevMonitorState.equals(r._monitorState.get(id))); t.equals(String(24), r._monitorState.get(id).get('value')); - t.equals(7, r._monitorState.get(id).get('moose')); + t.equals(7, r._monitorState.get(id).get('x')); t.end(); }); From 2867b5c331daeb9c16a59aa2205e38b5aa773ba2 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Wed, 24 May 2017 15:43:12 -0400 Subject: [PATCH 6/9] add record file --- src/engine/records.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 src/engine/records.js diff --git a/src/engine/records.js b/src/engine/records.js new file mode 100644 index 000000000..39f927f8a --- /dev/null +++ b/src/engine/records.js @@ -0,0 +1,12 @@ +const {Record} = require('immutable'); + +const MonitorRecord = Record({ + id: null, + category: 'data', + label: null, + value: null, + x: null, + y: null +}); + +module.exports = MonitorRecord; From 18282fc1d0925b91334138c70609506f78aafc36 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Wed, 24 May 2017 15:48:11 -0400 Subject: [PATCH 7/9] remove get for records --- src/engine/runtime.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 26dc4d354..495dd1695 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -883,7 +883,7 @@ class Runtime extends EventEmitter { * @param {!MonitorRecord} monitor Monitor to add. */ requestAddMonitor (monitor) { - this._monitorState = this._monitorState.set(monitor.get('id'), monitor); + this._monitorState = this._monitorState.set(monitor.id, monitor); } /** From 55f677702aaac827c38d8dadf4ba5f307ad7abb6 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Tue, 30 May 2017 10:05:13 -0400 Subject: [PATCH 8/9] add missing file --- src/engine/monitor-record.js | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 src/engine/monitor-record.js diff --git a/src/engine/monitor-record.js b/src/engine/monitor-record.js new file mode 100644 index 000000000..76ed08489 --- /dev/null +++ b/src/engine/monitor-record.js @@ -0,0 +1,10 @@ +const {Record} = require('immutable'); + +const MonitorRecord = Record({ + id: null, + opcode: null, + value: null, + params: null +}); + +module.exports = MonitorRecord; From 875eba568bc74f93a941d695f0c1ea94e2869dc4 Mon Sep 17 00:00:00 2001 From: DD Liu Date: Tue, 30 May 2017 10:24:09 -0400 Subject: [PATCH 9/9] fix file name in test --- test/unit/engine_runtime.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/engine_runtime.js b/test/unit/engine_runtime.js index 6ac6aaab0..458f3884c 100644 --- a/test/unit/engine_runtime.js +++ b/test/unit/engine_runtime.js @@ -1,6 +1,6 @@ const test = require('tap').test; const Runtime = require('../../src/engine/runtime'); -const MonitorRecord = require('../../src/engine/records'); +const MonitorRecord = require('../../src/engine/monitor-record'); const {Map} = require('immutable'); test('spec', t => {