From e3093ce67b9a1d175a1a73fc39dc8295fd41678c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 1 Nov 2025 11:25:43 +0000 Subject: [PATCH 1/3] refactor: Refactor sync modifiers to use shared Data1D processing logic - Extract common Data1D processing logic into processData1D() helper function - Refactor synccomputesmodifier.ts, syncfixesmodifier.ts, and syncvariablesmodifier.ts to use shared function - Reduce code duplication from ~125 lines to ~70-73 lines per file - Maintain all unique logic (execute checks, sync calls, name array handling) Closes #186 --- src/modifiers/modifier.ts | 85 ++++++++++++++++++++++++++ src/modifiers/synccomputesmodifier.ts | 80 ++++-------------------- src/modifiers/syncfixesmodifier.ts | 76 ++++------------------- src/modifiers/syncvariablesmodifier.ts | 76 ++++------------------- 4 files changed, 122 insertions(+), 195 deletions(-) diff --git a/src/modifiers/modifier.ts b/src/modifiers/modifier.ts index 4c0e43b..d276653 100644 --- a/src/modifiers/modifier.ts +++ b/src/modifiers/modifier.ts @@ -1,10 +1,95 @@ import { ModifierInput, ModifierOutput } from "./types"; +import { LMPModifier, Data1D } from "../types"; interface ModifierProps { name: string; active: boolean; } +/** + * Shared function to process Data1D from LMPModifier. + * Handles Data1D extraction, processing, and WASM memory cleanup. + */ +export function processData1D( + lmpModifier: LMPModifier, + data1D: Data1D | undefined, + input: ModifierInput, + everything: boolean, + syncDataPoints: boolean, + clearPerSync: boolean, +): { data1D: Data1D | undefined; hasData1D: boolean } { + // Get data1DNamesWrapper and extract size, then delete immediately + const data1DNamesWrapper = lmpModifier.getData1DNames(); + const hasData1D = data1DNamesWrapper.size() > 0; + const data1DNamesSize = data1DNamesWrapper.size(); + data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak + + if (data1DNamesSize === 0) { + return { data1D, hasData1D: false }; + } + + // Initialize data1D if needed + let currentData1D = data1D; + if (currentData1D == null) { + currentData1D = { + data: [], + labels: [], + }; + } + + if ((everything || syncDataPoints) && currentData1D) { + // Data points is only for plotting figures + if (clearPerSync) { + // For histograms (compute rdf etc) we don't have time as x axis, so we clear every time + currentData1D.data = []; + } + + const lengthBeforeWeStart = currentData1D.data.length; // Used to avoid copying all data every time + + if (currentData1D.labels.length === 0) { + // First label is never visible + currentData1D.labels.push("x"); + } + + // Get data1DVector once before the loop for better performance + const data1DVector = lmpModifier.getData1D(); + + for (let j = 0; j < data1DNamesSize; j++) { + const lmpData = data1DVector.get(j); + + if (currentData1D.labels.length - 1 === j) { + // Add missing labels + currentData1D.labels.push(lmpData.getLabel()); + } + + const xValuesPointer = lmpData.getXValuesPointer() / 4; + const yValuesPointer = lmpData.getYValuesPointer() / 4; + const xValues = input.wasm.HEAPF32.subarray( + xValuesPointer, + xValuesPointer + lmpData.getNumPoints(), + ) as Float32Array; + const yValues = input.wasm.HEAPF32.subarray( + yValuesPointer, + yValuesPointer + lmpData.getNumPoints(), + ) as Float32Array; + for (let k = lengthBeforeWeStart; k < xValues.length; k++) { + if (j === 0) { + currentData1D.data.push([xValues[k]]); + } + currentData1D.data[k].push(yValues[k]); + } + + // Delete the Data1D copy to prevent memory leak + lmpData.delete(); + } + + // Delete the vector wrapper after the loop to prevent memory leak + data1DVector.delete(); + } + + return { data1D: currentData1D, hasData1D }; +} + class Modifier { public name: string; public key: string; diff --git a/src/modifiers/synccomputesmodifier.ts b/src/modifiers/synccomputesmodifier.ts index 069df3c..c2b5e55 100644 --- a/src/modifiers/synccomputesmodifier.ts +++ b/src/modifiers/synccomputesmodifier.ts @@ -1,4 +1,5 @@ import Modifier from "./modifier"; +import { processData1D } from "./modifier"; import { ModifierInput, ModifierOutput } from "./types"; interface SyncComputesModifierProps { @@ -51,73 +52,18 @@ class SyncComputesModifier extends Modifier { compute.xLabel = compute.lmpCompute.getXLabel(); compute.yLabel = compute.lmpCompute.getYLabel(); compute.scalarValue = compute.lmpCompute.getScalarValue(); - - // Get data1DNamesWrapper and extract size, then delete immediately - const data1DNamesWrapper = compute.lmpCompute.getData1DNames(); - compute.hasData1D = data1DNamesWrapper.size() > 0; - const data1DNamesSize = data1DNamesWrapper.size(); - data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak - - if (data1DNamesSize > 0) { - compute.clearPerSync = compute.lmpCompute.getClearPerSync(); - - if (compute.data1D == null) { - compute.data1D = { - data: [], - labels: [], - }; - } - - if ((everything || compute.syncDataPoints) && compute.data1D) { - // Data points is only for plotting figures - if (compute.clearPerSync) { - // For histograms (compute rdf etc) we don't have time as x axis, so we clear every time - compute.data1D.data = []; - } - - const lengthBeforeWeStart = compute.data1D.data.length; // Used to avoid coping all data every time - - if (compute.data1D.labels.length === 0) { - // First label is never visible - compute.data1D.labels.push("x"); - } - - // Get data1DVector once before the loop for better performance - const data1DVector = compute.lmpCompute.getData1D(); - - for (let j = 0; j < data1DNamesSize; j++) { - const lmpData = data1DVector.get(j); - - if (compute.data1D.labels.length - 1 === j) { - // Add missing labels - compute.data1D.labels.push(lmpData.getLabel()); - } - - const xValuesPointer = lmpData.getXValuesPointer() / 4; - const yValuesPointer = lmpData.getYValuesPointer() / 4; - const xValues = input.wasm.HEAPF32.subarray( - xValuesPointer, - xValuesPointer + lmpData.getNumPoints(), - ) as Float32Array; - const yValues = input.wasm.HEAPF32.subarray( - yValuesPointer, - yValuesPointer + lmpData.getNumPoints(), - ) as Float32Array; - for (let k = lengthBeforeWeStart; k < xValues.length; k++) { - if (j === 0) { - compute.data1D.data.push([xValues[k]]); - } - compute.data1D.data[k].push(yValues[k]); - } - - // Delete the Data1D copy to prevent memory leak - lmpData.delete(); - } - - // Delete the vector wrapper after the loop to prevent memory leak - data1DVector.delete(); - } - } + compute.clearPerSync = compute.lmpCompute.getClearPerSync(); + + const { data1D, hasData1D } = processData1D( + compute.lmpCompute, + compute.data1D, + input, + everything, + compute.syncDataPoints, + compute.clearPerSync, + ); + compute.data1D = data1D; + compute.hasData1D = hasData1D; } output.computes[name] = compute; } diff --git a/src/modifiers/syncfixesmodifier.ts b/src/modifiers/syncfixesmodifier.ts index d1d5471..80d90c6 100644 --- a/src/modifiers/syncfixesmodifier.ts +++ b/src/modifiers/syncfixesmodifier.ts @@ -1,4 +1,5 @@ import Modifier from "./modifier"; +import { processData1D } from "./modifier"; import { ModifierInput, ModifierOutput } from "./types"; interface SyncFixesModifierProps { @@ -48,72 +49,19 @@ class SyncFixesModifier extends Modifier { fix.xLabel = fix.lmpFix.getXLabel(); fix.yLabel = fix.lmpFix.getYLabel(); fix.scalarValue = fix.lmpFix.getScalarValue(); + fix.clearPerSync = fix.lmpFix.getClearPerSync(); - // Get data1DNamesWrapper and extract size, then delete immediately - const data1DNamesWrapper = fix.lmpFix.getData1DNames(); - fix.hasData1D = data1DNamesWrapper.size() > 0; - const data1DNamesSize = data1DNamesWrapper.size(); - data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak + const { data1D, hasData1D } = processData1D( + fix.lmpFix, + fix.data1D, + input, + everything, + fix.syncDataPoints, + fix.clearPerSync, + ); + fix.data1D = data1D; + fix.hasData1D = hasData1D; - if (data1DNamesSize > 0) { - fix.clearPerSync = fix.lmpFix.getClearPerSync(); - if (fix.data1D == null) { - fix.data1D = { - data: [], - labels: [], - }; - } - - if ((everything || fix.syncDataPoints) && fix.data1D) { - // Data points is only for plotting figures - if (fix.clearPerSync) { - // For histograms (compute rdf etc) we don't have time as x axis, so we clear every time - fix.data1D.data = []; - } - - const lengthBeforeWeStart = fix.data1D.data.length; // Used to avoid coping all data every time - - if (fix.data1D.labels.length === 0) { - // First label is never visible - fix.data1D.labels.push("x"); - } - - // Get data1DVector once before the loop for better performance - const data1DVector = fix.lmpFix.getData1D(); - - for (let j = 0; j < data1DNamesSize; j++) { - const lmpData = data1DVector.get(j); - - if (fix.data1D.labels.length - 1 === j) { - // Add missing labels - fix.data1D.labels.push(lmpData.getLabel()); - } - - const xValuesPointer = lmpData.getXValuesPointer() / 4; - const yValuesPointer = lmpData.getYValuesPointer() / 4; - const xValues = input.wasm.HEAPF32.subarray( - xValuesPointer, - xValuesPointer + lmpData.getNumPoints(), - ) as Float32Array; - const yValues = input.wasm.HEAPF32.subarray( - yValuesPointer, - yValuesPointer + lmpData.getNumPoints(), - ) as Float32Array; - for (let k = lengthBeforeWeStart; k < xValues.length; k++) { - if (j === 0) { - fix.data1D.data.push([xValues[k]]); - } - fix.data1D.data[k].push(yValues[k]); - } - - // Delete the Data1D copy to prevent memory leak - lmpData.delete(); - } - - // Delete the vector wrapper after the loop to prevent memory leak - data1DVector.delete(); - } - } output.fixes[name] = fix; } }; diff --git a/src/modifiers/syncvariablesmodifier.ts b/src/modifiers/syncvariablesmodifier.ts index f84923d..67e0dda 100644 --- a/src/modifiers/syncvariablesmodifier.ts +++ b/src/modifiers/syncvariablesmodifier.ts @@ -1,4 +1,5 @@ import Modifier from "./modifier"; +import { processData1D } from "./modifier"; import { ModifierInput, ModifierOutput } from "./types"; interface SyncVariablesModifierProps { @@ -50,72 +51,19 @@ class SyncVariablesModifier extends Modifier { variable.yLabel = variable.lmpVariable.getYLabel(); variable.scalarValue = variable.lmpVariable.getScalarValue(); variable.hasScalarData = variable.lmpVariable.hasScalarData(); + variable.clearPerSync = variable.lmpVariable.getClearPerSync(); - // Get data1DNamesWrapper and extract size, then delete immediately - const data1DNamesWrapper = variable.lmpVariable.getData1DNames(); - variable.hasData1D = data1DNamesWrapper.size() > 0; - const data1DNamesSize = data1DNamesWrapper.size(); - data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak - - if (data1DNamesSize > 0) { - variable.clearPerSync = variable.lmpVariable.getClearPerSync(); - if (variable.data1D == null) { - variable.data1D = { - data: [], - labels: [], - }; - } + const { data1D, hasData1D } = processData1D( + variable.lmpVariable, + variable.data1D, + input, + everything, + variable.syncDataPoints, + variable.clearPerSync, + ); + variable.data1D = data1D; + variable.hasData1D = hasData1D; - if ((everything || variable.syncDataPoints) && variable.data1D) { - // Data points is only for plotting figures - if (variable.clearPerSync) { - // For histograms (compute rdf etc) we don't have time as x axis, so we clear every time - variable.data1D.data = []; - } - - const lengthBeforeWeStart = variable.data1D.data.length; // Used to avoid coping all data every time - - if (variable.data1D.labels.length === 0) { - // First label is never visible - variable.data1D.labels.push("x"); - } - - // Get data1DVector once before the loop for better performance - const data1DVector = variable.lmpVariable.getData1D(); - - for (let j = 0; j < data1DNamesSize; j++) { - const lmpData = data1DVector.get(j); - - if (variable.data1D.labels.length - 1 === j) { - // Add missing labels - variable.data1D.labels.push(lmpData.getLabel()); - } - - const xValuesPointer = lmpData.getXValuesPointer() / 4; - const yValuesPointer = lmpData.getYValuesPointer() / 4; - const xValues = input.wasm.HEAPF32.subarray( - xValuesPointer, - xValuesPointer + lmpData.getNumPoints(), - ) as Float32Array; - const yValues = input.wasm.HEAPF32.subarray( - yValuesPointer, - yValuesPointer + lmpData.getNumPoints(), - ) as Float32Array; - for (let k = lengthBeforeWeStart; k < xValues.length; k++) { - if (j === 0) { - variable.data1D.data.push([xValues[k]]); - } - variable.data1D.data[k].push(yValues[k]); - } - - // Delete the Data1D copy to prevent memory leak - lmpData.delete(); - } - - // Delete the vector wrapper after the loop to prevent memory leak - data1DVector.delete(); - } - } output.variables[name] = variable; } }; From d13436e640ceaf998c072e890c42372d789ff28f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 1 Nov 2025 11:40:02 +0000 Subject: [PATCH 2/3] refactor: Address PR review comments for processData1D function - Optimize size() call: store data1DNamesSize once instead of calling twice - Remove redundant check: currentData1D is guaranteed to be an object - Replace magic number 4 with Float32Array.BYTES_PER_ELEMENT for clarity --- src/modifiers/modifier.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/modifiers/modifier.ts b/src/modifiers/modifier.ts index d276653..38463ee 100644 --- a/src/modifiers/modifier.ts +++ b/src/modifiers/modifier.ts @@ -20,8 +20,8 @@ export function processData1D( ): { data1D: Data1D | undefined; hasData1D: boolean } { // Get data1DNamesWrapper and extract size, then delete immediately const data1DNamesWrapper = lmpModifier.getData1DNames(); - const hasData1D = data1DNamesWrapper.size() > 0; const data1DNamesSize = data1DNamesWrapper.size(); + const hasData1D = data1DNamesSize > 0; data1DNamesWrapper.delete(); // Delete WASM wrapper to prevent memory leak if (data1DNamesSize === 0) { @@ -37,7 +37,7 @@ export function processData1D( }; } - if ((everything || syncDataPoints) && currentData1D) { + if (everything || syncDataPoints) { // Data points is only for plotting figures if (clearPerSync) { // For histograms (compute rdf etc) we don't have time as x axis, so we clear every time @@ -62,8 +62,8 @@ export function processData1D( currentData1D.labels.push(lmpData.getLabel()); } - const xValuesPointer = lmpData.getXValuesPointer() / 4; - const yValuesPointer = lmpData.getYValuesPointer() / 4; + const xValuesPointer = lmpData.getXValuesPointer() / Float32Array.BYTES_PER_ELEMENT; + const yValuesPointer = lmpData.getYValuesPointer() / Float32Array.BYTES_PER_ELEMENT; const xValues = input.wasm.HEAPF32.subarray( xValuesPointer, xValuesPointer + lmpData.getNumPoints(), From 47e8f17169389112d964d910e50811511415c742 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 1 Nov 2025 11:53:14 +0000 Subject: [PATCH 3/3] refactor: Improve readability of data processing loop in processData1D Use if/else block to clearly separate logic for initializing new row (j === 0) from appending to existing row (j > 0) --- src/modifiers/modifier.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/modifiers/modifier.ts b/src/modifiers/modifier.ts index 38463ee..d282922 100644 --- a/src/modifiers/modifier.ts +++ b/src/modifiers/modifier.ts @@ -74,9 +74,10 @@ export function processData1D( ) as Float32Array; for (let k = lengthBeforeWeStart; k < xValues.length; k++) { if (j === 0) { - currentData1D.data.push([xValues[k]]); + currentData1D.data.push([xValues[k], yValues[k]]); + } else { + currentData1D.data[k].push(yValues[k]); } - currentData1D.data[k].push(yValues[k]); } // Delete the Data1D copy to prevent memory leak