From f98c34b856e70aaecd307ee9ee6406803c51a7f4 Mon Sep 17 00:00:00 2001 From: Can Ince Date: Wed, 20 Mar 2024 06:55:14 +0000 Subject: [PATCH 01/13] Add safeguards to activateRoots --- runtime/elem/Runtime.h | 57 ++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/runtime/elem/Runtime.h b/runtime/elem/Runtime.h index 051393e..c0315ef 100644 --- a/runtime/elem/Runtime.h +++ b/runtime/elem/Runtime.h @@ -332,42 +332,67 @@ namespace elem // Populate and activate from the incoming event std::set active; - for (auto const& v : roots) { + for (auto const& v : roots) + { if (!v.isNumber()) + { + ELEM_DBG("[Error] activateRoot - Invalid nodeId format."); return ReturnCode::InvalidInstructionFormat(); + } - int32_t nodeId = static_cast((js::Number) v); + int32_t nodeId = static_cast((js::Number)v); ELEM_DBG("[Native] activateRoot " << nodeIdToHex(nodeId)); - if (nodeTable.find(nodeId) == nodeTable.end()) + // Using find() method for safe access to nodeTable elements + auto it = nodeTable.find(nodeId); + if (it == nodeTable.end()) + { + ELEM_DBG("[Error] activateRoot - NodeId not found: " << nodeIdToHex(nodeId)); return ReturnCode::NodeNotFound(); + } - if (auto ptr = std::dynamic_pointer_cast>(nodeTable.at(nodeId))) { + // Attempt to cast the found node to a RootNode type and activate it + auto ptr = std::dynamic_pointer_cast>(it->second); + if (ptr) + { ptr->setProperty("active", true); active.insert(nodeId); + ELEM_DBG("[Success] Activated root: " << nodeIdToHex(nodeId)); + } + else + { + ELEM_DBG("[Error] Failed to cast to RootNode or activate: " << nodeIdToHex(nodeId)); } } - // Deactivate any prior roots - for (auto const& n : currentRoots) { - if (auto ptr = std::dynamic_pointer_cast>(nodeTable.at(n))) { - // If any current root was not marked active in this event, we deactivate it - if (active.count(n) == 0) { - ptr->setProperty("active", false); - } + // Deactivate any prior roots not included in the incoming active set + for (auto const& n : currentRoots) + { + auto it = nodeTable.find(n); + if (it != nodeTable.end()) + { + auto ptr = std::dynamic_pointer_cast>(it->second); + if (ptr) + { + if (active.count(n) == 0) + { + ptr->setProperty("active", false); + ELEM_DBG("[Success] Deactivated root: " << nodeIdToHex(n)); + } - // And if it's still running, we hang onto it - if (ptr->stillRunning()) { - active.insert(n); + if (ptr->stillRunning()) + { + active.insert(n); + } } } } - // Merge - currentRoots = active; + currentRoots.swap(active); // More efficient than assignment return ReturnCode::Ok(); } + //============================================================================== template void Runtime::processQueuedEvents(std::function&& evtCallback) From 95f04d8d8965d3fd81b29a73966394fa6d8e4c97 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Thu, 21 Mar 2024 10:22:43 -0700 Subject: [PATCH 02/13] fix: GC cutting off tails --- js/packages/core/index.ts | 32 ++++++++--------------------- js/packages/core/src/Reconciler.res | 10 +++++---- 2 files changed, 15 insertions(+), 27 deletions(-) diff --git a/js/packages/core/index.ts b/js/packages/core/index.ts index 82e8ad2..8ade1c2 100644 --- a/js/packages/core/index.ts +++ b/js/packages/core/index.ts @@ -57,7 +57,7 @@ class Delegate { private currentActiveRoots: Set; private terminalGeneration: number; - private batch: any; + private batch: any[]; constructor(terminalGeneration = 8) { this.nodeMap = new Map(); @@ -73,14 +73,7 @@ class Delegate { this.edgesAdded = 0; this.propsWritten = 0; - this.batch = { - createNode: [], - deleteNode: [], - appendChild: [], - setProperty: [], - activateRoots: [], - commitUpdates: [], - }; + this.batch = [] } getNodeMap() { return this.nodeMap; } @@ -88,22 +81,22 @@ class Delegate { createNode(hash, type) { this.nodesAdded++; - this.batch.createNode.push([InstructionTypes.CREATE_NODE, hash, type]); + this.batch.push([InstructionTypes.CREATE_NODE, hash, type]); } deleteNode(hash) { this.nodesRemoved++; - this.batch.deleteNode.push([InstructionTypes.DELETE_NODE, hash]); + this.batch.push([InstructionTypes.DELETE_NODE, hash]); } appendChild(parentHash, childHash) { this.edgesAdded++; - this.batch.appendChild.push([InstructionTypes.APPEND_CHILD, parentHash, childHash]); + this.batch.push([InstructionTypes.APPEND_CHILD, parentHash, childHash]); } setProperty(hash, key, value) { this.propsWritten++; - this.batch.setProperty.push([InstructionTypes.SET_PROPERTY, hash, key, value]); + this.batch.push([InstructionTypes.SET_PROPERTY, hash, key, value]); } activateRoots(roots) { @@ -116,24 +109,17 @@ class Delegate { roots.every((root) => this.currentActiveRoots.has(root)); if (!alreadyActive) { - this.batch.activateRoots.push([InstructionTypes.ACTIVATE_ROOTS, roots]); + this.batch.push([InstructionTypes.ACTIVATE_ROOTS, roots]); this.currentActiveRoots = new Set(roots); } } commitUpdates() { - this.batch.commitUpdates.push([InstructionTypes.COMMIT_UPDATES]); + this.batch.push([InstructionTypes.COMMIT_UPDATES]); } getPackedInstructions() { - return [ - ...this.batch.createNode, - ...this.batch.deleteNode, - ...this.batch.appendChild, - ...this.batch.setProperty, - ...this.batch.activateRoots, - ...this.batch.commitUpdates, - ]; + return this.batch; } } diff --git a/js/packages/core/src/Reconciler.res b/js/packages/core/src/Reconciler.res index e1f2d0f..e6ef964 100644 --- a/js/packages/core/src/Reconciler.res +++ b/js/packages/core/src/Reconciler.res @@ -131,13 +131,15 @@ let renderWithDelegate = (delegate, graphs) => { visit(delegate, visitSet, roots) - if (RenderDelegate.getTerminalGeneration(delegate) > 1) { - stepGarbageCollector(delegate) - } - RenderDelegate.activateRoots(delegate, Belt.List.toArray(Belt.List.map(roots, r => r.hash))) // TODO: transaction semantics! RenderDelegate.commitUpdates(delegate) + + // Garbage collect after render, + // So we don't delete an inactive node before the audio runtime has a ref to it + if (RenderDelegate.getTerminalGeneration(delegate) > 1) { + stepGarbageCollector(delegate) + } } From 0e89a8b4416c9caa7f41c737f6e7878d30e0af47 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Thu, 21 Mar 2024 10:33:47 -0700 Subject: [PATCH 03/13] revert: just change instruction order manually --- js/packages/core/index.ts | 32 +++++++++++++++++++++-------- js/packages/core/src/Reconciler.res | 2 -- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/js/packages/core/index.ts b/js/packages/core/index.ts index 8ade1c2..8281d64 100644 --- a/js/packages/core/index.ts +++ b/js/packages/core/index.ts @@ -57,7 +57,7 @@ class Delegate { private currentActiveRoots: Set; private terminalGeneration: number; - private batch: any[]; + private batch: any; constructor(terminalGeneration = 8) { this.nodeMap = new Map(); @@ -73,7 +73,14 @@ class Delegate { this.edgesAdded = 0; this.propsWritten = 0; - this.batch = [] + this.batch = { + createNode: [], + appendChild: [], + setProperty: [], + activateRoots: [], + commitUpdates: [], + deleteNode: [] + }; } getNodeMap() { return this.nodeMap; } @@ -81,22 +88,22 @@ class Delegate { createNode(hash, type) { this.nodesAdded++; - this.batch.push([InstructionTypes.CREATE_NODE, hash, type]); + this.batch.createNode.push([InstructionTypes.CREATE_NODE, hash, type]); } deleteNode(hash) { this.nodesRemoved++; - this.batch.push([InstructionTypes.DELETE_NODE, hash]); + this.batch.deleteNode.push([InstructionTypes.DELETE_NODE, hash]); } appendChild(parentHash, childHash) { this.edgesAdded++; - this.batch.push([InstructionTypes.APPEND_CHILD, parentHash, childHash]); + this.batch.appendChild.push([InstructionTypes.APPEND_CHILD, parentHash, childHash]); } setProperty(hash, key, value) { this.propsWritten++; - this.batch.push([InstructionTypes.SET_PROPERTY, hash, key, value]); + this.batch.setProperty.push([InstructionTypes.SET_PROPERTY, hash, key, value]); } activateRoots(roots) { @@ -109,17 +116,24 @@ class Delegate { roots.every((root) => this.currentActiveRoots.has(root)); if (!alreadyActive) { - this.batch.push([InstructionTypes.ACTIVATE_ROOTS, roots]); + this.batch.activateRoots.push([InstructionTypes.ACTIVATE_ROOTS, roots]); this.currentActiveRoots = new Set(roots); } } commitUpdates() { - this.batch.push([InstructionTypes.COMMIT_UPDATES]); + this.batch.commitUpdates.push([InstructionTypes.COMMIT_UPDATES]); } getPackedInstructions() { - return this.batch; + return [ + ...this.batch.createNode, + ...this.batch.appendChild, + ...this.batch.setProperty, + ...this.batch.activateRoots, + ...this.batch.commitUpdates, + ...this.batch.deleteNode, + ]; } } diff --git a/js/packages/core/src/Reconciler.res b/js/packages/core/src/Reconciler.res index e6ef964..c2934f1 100644 --- a/js/packages/core/src/Reconciler.res +++ b/js/packages/core/src/Reconciler.res @@ -136,8 +136,6 @@ let renderWithDelegate = (delegate, graphs) => { // TODO: transaction semantics! RenderDelegate.commitUpdates(delegate) - // Garbage collect after render, - // So we don't delete an inactive node before the audio runtime has a ref to it if (RenderDelegate.getTerminalGeneration(delegate) > 1) { stepGarbageCollector(delegate) } From 356148ee2cb693523fba5c4b04f9dca082ac84b1 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Thu, 21 Mar 2024 10:35:18 -0700 Subject: [PATCH 04/13] feat: minimal changes --- js/packages/core/src/Reconciler.res | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/js/packages/core/src/Reconciler.res b/js/packages/core/src/Reconciler.res index c2934f1..e1f2d0f 100644 --- a/js/packages/core/src/Reconciler.res +++ b/js/packages/core/src/Reconciler.res @@ -131,13 +131,13 @@ let renderWithDelegate = (delegate, graphs) => { visit(delegate, visitSet, roots) + if (RenderDelegate.getTerminalGeneration(delegate) > 1) { + stepGarbageCollector(delegate) + } + RenderDelegate.activateRoots(delegate, Belt.List.toArray(Belt.List.map(roots, r => r.hash))) // TODO: transaction semantics! RenderDelegate.commitUpdates(delegate) - - if (RenderDelegate.getTerminalGeneration(delegate) > 1) { - stepGarbageCollector(delegate) - } } From 7b7bf089a39e5214688a0b95982b122012f013f1 Mon Sep 17 00:00:00 2001 From: Can Ince Date: Tue, 26 Mar 2024 17:01:23 +0000 Subject: [PATCH 05/13] Bump gitignore --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index b2ed1d2..599215f 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,8 @@ node_modules js/**/lib/bs js/packages/**/binaries/ js/packages/**/*.min.js +/runtime/.DS_Store +/js/packages/core/.DS_Store +/js/packages/.DS_Store +/js/.DS_Store +/.DS_Store From bd326b6e039640dadc8b8d7522b05c4df9bce142 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Tue, 26 Mar 2024 22:41:14 -0700 Subject: [PATCH 06/13] optimize: remove hash table lookup from render ops --- runtime/elem/GraphRenderSequence.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/runtime/elem/GraphRenderSequence.h b/runtime/elem/GraphRenderSequence.h index 9432fd5..518545a 100644 --- a/runtime/elem/GraphRenderSequence.h +++ b/runtime/elem/GraphRenderSequence.h @@ -92,9 +92,9 @@ namespace elem // Next we prepare the render operation bufferMap.emplace(node->getId(), ba.next()); + auto* outputData = bufferMap.at(node->getId()); - renderOps.push_back([=, bufferMap = this->bufferMap](HostContext& ctx) { - auto* outputData = bufferMap.at(node->getId()); + renderOps.push_back([=](HostContext& ctx) { node->process(BlockContext { ctx.inputData, @@ -117,18 +117,18 @@ namespace elem } // Next we prepare the render operation - bufferMap.emplace(node->getId(), ba.next()); + auto* outputData ba.next(); + bufferMap.emplace(node->getId(), outputData); // Allocate room for the child pointers here, gets moved into the lambda capture group below std::vector ptrs(children.size()); + auto const numChildren = children.size(); - renderOps.push_back([=, bufferMap = this->bufferMap, ptrs = std::move(ptrs)](HostContext& ctx) mutable { - auto* outputData = bufferMap.at(node->getId()); - auto const numChildren = children.size(); + for (size_t j = 0; j < numChildren; ++j) { + ptrs[j] = bufferMap.at(children[j]); + } - for (size_t j = 0; j < numChildren; ++j) { - ptrs[j] = bufferMap.at(children[j]); - } + renderOps.push_back([=, ptrs = std::move(ptrs)](HostContext& ctx) mutable { node->process(BlockContext { const_cast(ptrs.data()), From ec1f98010e04b0cc5c31f79e6f70c551d9fd5d92 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Tue, 26 Mar 2024 23:01:00 -0700 Subject: [PATCH 07/13] bump --- runtime/elem/GraphRenderSequence.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/elem/GraphRenderSequence.h b/runtime/elem/GraphRenderSequence.h index 518545a..eae39e7 100644 --- a/runtime/elem/GraphRenderSequence.h +++ b/runtime/elem/GraphRenderSequence.h @@ -117,7 +117,7 @@ namespace elem } // Next we prepare the render operation - auto* outputData ba.next(); + auto* outputData = ba.next(); bufferMap.emplace(node->getId(), outputData); // Allocate room for the child pointers here, gets moved into the lambda capture group below From b3a03273e2b861afc738bf31b789b7d7499db4ec Mon Sep 17 00:00:00 2001 From: Nathan Blair Date: Wed, 27 Mar 2024 08:24:40 -0700 Subject: [PATCH 08/13] optimize: remove hash table lookup from render ops (#3) * optimize: remove hash table lookup from render ops * bump --- runtime/elem/GraphRenderSequence.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/runtime/elem/GraphRenderSequence.h b/runtime/elem/GraphRenderSequence.h index 9432fd5..eae39e7 100644 --- a/runtime/elem/GraphRenderSequence.h +++ b/runtime/elem/GraphRenderSequence.h @@ -92,9 +92,9 @@ namespace elem // Next we prepare the render operation bufferMap.emplace(node->getId(), ba.next()); + auto* outputData = bufferMap.at(node->getId()); - renderOps.push_back([=, bufferMap = this->bufferMap](HostContext& ctx) { - auto* outputData = bufferMap.at(node->getId()); + renderOps.push_back([=](HostContext& ctx) { node->process(BlockContext { ctx.inputData, @@ -117,18 +117,18 @@ namespace elem } // Next we prepare the render operation - bufferMap.emplace(node->getId(), ba.next()); + auto* outputData = ba.next(); + bufferMap.emplace(node->getId(), outputData); // Allocate room for the child pointers here, gets moved into the lambda capture group below std::vector ptrs(children.size()); + auto const numChildren = children.size(); - renderOps.push_back([=, bufferMap = this->bufferMap, ptrs = std::move(ptrs)](HostContext& ctx) mutable { - auto* outputData = bufferMap.at(node->getId()); - auto const numChildren = children.size(); + for (size_t j = 0; j < numChildren; ++j) { + ptrs[j] = bufferMap.at(children[j]); + } - for (size_t j = 0; j < numChildren; ++j) { - ptrs[j] = bufferMap.at(children[j]); - } + renderOps.push_back([=, ptrs = std::move(ptrs)](HostContext& ctx) mutable { node->process(BlockContext { const_cast(ptrs.data()), From adc2b731b85a09cb6cdc5483debda64076525246 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Tue, 2 Apr 2024 15:52:47 -0700 Subject: [PATCH 09/13] fix: render first sequence with no crossfade --- runtime/elem/Runtime.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/runtime/elem/Runtime.h b/runtime/elem/Runtime.h index c0315ef..46e2d66 100644 --- a/runtime/elem/Runtime.h +++ b/runtime/elem/Runtime.h @@ -356,6 +356,10 @@ namespace elem if (ptr) { ptr->setProperty("active", true); + if (currentRoots.empty()) + { + ptr->currentGain = FloatType(1); + } active.insert(nodeId); ELEM_DBG("[Success] Activated root: " << nodeIdToHex(nodeId)); } From 2500de62b71563d76a729278a76a4d3b052d46b5 Mon Sep 17 00:00:00 2001 From: Ncblair Date: Wed, 3 Apr 2024 11:24:19 -0700 Subject: [PATCH 10/13] tidy --- runtime/elem/Runtime.h | 6 +----- runtime/elem/builtins/Core.h | 6 ++++++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/runtime/elem/Runtime.h b/runtime/elem/Runtime.h index 46e2d66..c697dd8 100644 --- a/runtime/elem/Runtime.h +++ b/runtime/elem/Runtime.h @@ -355,11 +355,7 @@ namespace elem auto ptr = std::dynamic_pointer_cast>(it->second); if (ptr) { - ptr->setProperty("active", true); - if (currentRoots.empty()) - { - ptr->currentGain = FloatType(1); - } + ptr->activate(currentRoots.empty() ? FloatType(1) : FloatType(0)); active.insert(nodeId); ELEM_DBG("[Success] Activated root: " << nodeIdToHex(nodeId)); } diff --git a/runtime/elem/builtins/Core.h b/runtime/elem/builtins/Core.h index 89ce2f5..f0845c6 100644 --- a/runtime/elem/builtins/Core.h +++ b/runtime/elem/builtins/Core.h @@ -32,6 +32,12 @@ namespace elem return (t >= 0.5 || (std::abs(c - t) >= std::numeric_limits::epsilon())); } + void activate(FloatType initialGain = FloatType(0)) + { + setProperty("active", true); + currentGain.store(initialGain); + } + int setProperty(std::string const& key, js::Value const& val) override { if (key == "active") { From 93596d4c230b80e07d7e7f2602020e3aa92f713a Mon Sep 17 00:00:00 2001 From: Can Ince Date: Sun, 14 Apr 2024 06:43:19 +0100 Subject: [PATCH 11/13] Updates for PR feedback --- .gitignore | 3 ++- runtime/elem/Runtime.h | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 599215f..7298889 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ node_modules .cljs_node_repl .cpcache .cache +.DS_Store *.swp @@ -16,4 +17,4 @@ js/packages/**/*.min.js /js/packages/core/.DS_Store /js/packages/.DS_Store /js/.DS_Store -/.DS_Store + diff --git a/runtime/elem/Runtime.h b/runtime/elem/Runtime.h index c697dd8..b6a1b06 100644 --- a/runtime/elem/Runtime.h +++ b/runtime/elem/Runtime.h @@ -372,6 +372,7 @@ namespace elem if (it != nodeTable.end()) { auto ptr = std::dynamic_pointer_cast>(it->second); + // If any current root was not marked active in this event, we deactivate it if (ptr) { if (active.count(n) == 0) @@ -379,7 +380,7 @@ namespace elem ptr->setProperty("active", false); ELEM_DBG("[Success] Deactivated root: " << nodeIdToHex(n)); } - + // And if it's still running, we hang onto it if (ptr->stillRunning()) { active.insert(n); @@ -387,8 +388,8 @@ namespace elem } } } - - currentRoots.swap(active); // More efficient than assignment + // Merge + currentRoots.swap(active); return ReturnCode::Ok(); } From 257b938c24e5c9109930172057168d6651898f73 Mon Sep 17 00:00:00 2001 From: Can Ince Date: Sun, 14 Apr 2024 06:51:55 +0100 Subject: [PATCH 12/13] Bump --- .gitignore | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.gitignore b/.gitignore index 7298889..4cf90c5 100644 --- a/.gitignore +++ b/.gitignore @@ -13,8 +13,3 @@ node_modules js/**/lib/bs js/packages/**/binaries/ js/packages/**/*.min.js -/runtime/.DS_Store -/js/packages/core/.DS_Store -/js/packages/.DS_Store -/js/.DS_Store - From f553dd3223dd406ade8d58a0841c5363ef479aed Mon Sep 17 00:00:00 2001 From: Can Ince Date: Sun, 14 Apr 2024 18:16:47 +0100 Subject: [PATCH 13/13] Update snap --- js/packages/core/__tests__/__snapshots__/core.test.js.snap | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/js/packages/core/__tests__/__snapshots__/core.test.js.snap b/js/packages/core/__tests__/__snapshots__/core.test.js.snap index 3e98a21..24fa4ca 100644 --- a/js/packages/core/__tests__/__snapshots__/core.test.js.snap +++ b/js/packages/core/__tests__/__snapshots__/core.test.js.snap @@ -529,6 +529,9 @@ exports[`garbage collection 1`] = ` exports[`garbage collection 2`] = ` [ + [ + 5, + ], [ 1, 520300429, @@ -537,9 +540,6 @@ exports[`garbage collection 2`] = ` 1, 1423955117, ], - [ - 5, - ], ] `;