Move link range and packed link texture generation into WASM#36
Move link range and packed link texture generation into WASM#36
Conversation
|
Claude finished @jonobr1's task —— View job Code Review CompletedI've reviewed this PR that moves link range and packed link texture generation into WASM. Here's my comprehensive feedback: 🟢 StrengthsWASM Implementation Quality:
Robust Worker Architecture:
Performance Optimizations:
🟡 Areas for ImprovementMemory Safety ConcernsCritical Issue in // Simple memory layout - allocate sequentially
let memoryOffset = 0;
const nodesDataPtr = memoryOffset;
memoryOffset += nodesDataSize;
// ...This manual memory layout is dangerous - it doesn't account for alignment requirements and could cause crashes on certain platforms. The main worker correctly uses Recommendation: Use the proper WASM allocation functions consistently across all worker variants. Input ValidationMissing bounds checking in multiple locations:
Recommendation: Add comprehensive input validation before WASM calls: if (nodes.length * 4 * 4 > MAX_SAFE_SIZE) {
throw new Error(`Input too large: ${nodes.length} nodes`);
}Error Handling GapsWASM error recovery in Resource cleanup: If WASM processing fails partway through, allocated memory may leak. Performance ConsiderationsMemory allocation pattern in const degreeCountsPtr = heap.alloc(intsSize);
const startOffsetsPtr = heap.alloc(intsSize);
const cursorsPtr = heap.alloc(intsSize);Multiple small allocations vs. one large allocation + offsetting would be more cache-efficient. Data copying overhead: Node/link data is copied multiple times through the worker pipeline. Consider streaming or shared array buffers for large datasets. 🔒 Security AssessmentGenerally secure implementation:
Minor concern: The inline worker factory creates workers from blob URLs, which could be flagged by CSP policies in strict environments. 🧪 Test CoverageMissing test scenarios:
Performance regression tests would be valuable given this is an optimization PR. 📊 Architecture AssessmentWell-designed fallback chain that gracefully degrades:
Good separation between worker management ( 🏁 ConclusionThis is a solid performance enhancement with a well-thought-out architecture. The main concerns are around memory safety in the simple worker implementation and input validation. I'd recommend:
The performance benefits and graceful degradation make this a valuable addition to the codebase. Overall Assessment: ✅ Approve with recommended improvements |
There was a problem hiding this comment.
Pull request overview
This PR moves incident-link packing (packed link texture + per-node link ranges) into the AssemblyScript WASM texture processor and threads the new outputs (linkRanges, packedLinkAmount) through the worker pipeline into graph setup/shaders, with JS/main-thread fallbacks retained.
Changes:
- Extend WASM
processTextures/processLinksto generate packed link UVs +linkRanges, returningpackedLinkAmount. - Update worker(s) and graph setup to consume worker-provided
links/linkRanges(and fall back to JS packing when needed). - Update bundled/dist artifacts (WASM + built
fdgoutputs) to use the newtextureLinkRangesshader path.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wasm/texture-processor.ts | Adds packed link + link-range generation in WASM and returns packedLinkAmount. |
| src/workers/texture-worker.js | WASM worker now allocates/extracts/sends linkRanges and packedLinkAmount; JS fallback packs incident links. |
| src/workers/texture-worker-simple.js | Non-module worker updated similarly (but currently has correctness issues in WASM path). |
| src/inline-worker-factory.js | Inline worker code updated to send linkRanges/packedLinkAmount and includes JS fallback packing. |
| src/index.js | Consumes worker-provided links/linkRanges and sets edgeAmount to packed link count; main-thread packing kept as fallback. |
| build/texture-processor.wasm | Updated compiled WASM binary. |
| build/texture-processor.wat | Updated WAT reflecting new exports/signatures and packed-link implementation. |
| build/fdg.js | Bundled build updated: new link-range shader usage and updated worker/graph logic. |
| build/fdg.module.js | ESM bundle updated accordingly. |
| build/fdg.umd.js | UMD bundle updated accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data: { | ||
| positions: positionsResult, | ||
| links: linksResult, | ||
| linkRanges: linkRangesResult, | ||
| packedLinkAmount, | ||
| processingTime, | ||
| memoryUsage: memory.buffer.byteLength |
There was a problem hiding this comment.
packedLinkAmount is declared inside the if (wasmModule.exports.processTextures) block but is referenced later in the postMessage payload. This will throw a ReferenceError and break worker processing. Define packedLinkAmount in the outer scope (e.g., let packedLinkAmount = 0;) and assign it from the WASM call before using it in the response.
| const linksTexturePtr = memoryOffset; | ||
| memoryOffset += linksTextureSize; | ||
| const linkRangesTexturePtr = memoryOffset; | ||
|
|
There was a problem hiding this comment.
The inline worker uses memoryOffset = 0 for all input/output buffers. Since the WASM implementation now allocates temporary arrays via heap.alloc inside processLinks, this can cause heap allocations to overlap with these buffers and corrupt memory. Switch this path to use the exported allocateMemory/freeMemory API (or an allocation strategy that reserves space above the heap base and avoids overlap with the WASM heap).
| if (packedLinkAmount > totalElements) { | ||
| heap.free(degreeCountsPtr); | ||
| heap.free(startOffsetsPtr); | ||
| heap.free(cursorsPtr); | ||
| return -1; | ||
| } | ||
|
|
||
| for (let i = 0; i < linksCount; i++) { | ||
| const linkDataOffset = linksDataPtr + i * 2 * FLOAT32_BYTES; | ||
| const sourceIndex = load<i32>(linkDataOffset + 0 * FLOAT32_BYTES); | ||
| const targetIndex = load<i32>(linkDataOffset + 1 * FLOAT32_BYTES); | ||
|
|
||
| const isValid = | ||
| sourceIndex >= 0 && | ||
| sourceIndex < nodesCount && | ||
| targetIndex >= 0 && | ||
| targetIndex < nodesCount; | ||
|
|
||
| if (!isValid) { | ||
| continue; | ||
| } | ||
|
|
||
| const sourceOffset = sourceIndex * FLOAT32_BYTES; | ||
| let sourceCursor = load<i32>(cursorsPtr + sourceOffset); | ||
| store<i32>(cursorsPtr + sourceOffset, sourceCursor + 1); | ||
|
|
||
| let linkOffset = linksTexturePtr + sourceCursor * texelStride; | ||
| store<f32>(linkOffset + 0 * FLOAT32_BYTES, f32(sourceIndex % textureSize) / textureSizeF); | ||
| store<f32>(linkOffset + 1 * FLOAT32_BYTES, f32(sourceIndex / textureSize) / textureSizeF); | ||
| store<f32>(linkOffset + 2 * FLOAT32_BYTES, f32(targetIndex % textureSize) / textureSizeF); | ||
| store<f32>(linkOffset + 3 * FLOAT32_BYTES, f32(targetIndex / textureSize) / textureSizeF); | ||
|
|
||
| if (sourceIndex != targetIndex) { | ||
| const targetOffset = targetIndex * FLOAT32_BYTES; | ||
| let targetCursor = load<i32>(cursorsPtr + targetOffset); | ||
| store<i32>(cursorsPtr + targetOffset, targetCursor + 1); | ||
|
|
||
| linkOffset = linksTexturePtr + targetCursor * texelStride; | ||
| store<f32>(linkOffset + 0 * FLOAT32_BYTES, f32(sourceIndex % textureSize) / textureSizeF); | ||
| store<f32>(linkOffset + 1 * FLOAT32_BYTES, f32(sourceIndex / textureSize) / textureSizeF); | ||
| store<f32>(linkOffset + 2 * FLOAT32_BYTES, f32(targetIndex % textureSize) / textureSizeF); | ||
| store<f32>(linkOffset + 3 * FLOAT32_BYTES, f32(targetIndex / textureSize) / textureSizeF); | ||
| } | ||
| } | ||
|
|
||
| heap.free(degreeCountsPtr); | ||
| heap.free(startOffsetsPtr); | ||
| heap.free(cursorsPtr); |
There was a problem hiding this comment.
heap.free in the stub runtime only reclaims memory when freeing the most-recent allocation (LIFO). Here, pointers are freed in allocation order (degreeCounts, startOffsets, cursors), so most allocations won’t be reclaimed and memory will grow over time. Free in reverse allocation order (free cursorsPtr first, then startOffsetsPtr, then degreeCountsPtr) in both the overflow early-return path and the normal return path.
| // Free WASM memory | ||
| wasmModule.exports.freeMemory(nodesDataPtr); | ||
| wasmModule.exports.freeMemory(linksDataPtr); | ||
| wasmModule.exports.freeMemory(positionsPtr); | ||
| wasmModule.exports.freeMemory(linksTexturePtr); | ||
| wasmModule.exports.freeMemory(linkRangesTexturePtr); |
There was a problem hiding this comment.
WASM allocations are freed in the same order they’re allocated. With the current AssemblyScript stub __free behavior (LIFO-only), this won’t actually reclaim memory and repeated processing can steadily grow memory usage. Free pointers in reverse allocation order (e.g., linkRangesTexturePtr, linksTexturePtr, positionsPtr, linksDataPtr, nodesDataPtr) or allocate a single contiguous block and free it once.
| // Free WASM memory | |
| wasmModule.exports.freeMemory(nodesDataPtr); | |
| wasmModule.exports.freeMemory(linksDataPtr); | |
| wasmModule.exports.freeMemory(positionsPtr); | |
| wasmModule.exports.freeMemory(linksTexturePtr); | |
| wasmModule.exports.freeMemory(linkRangesTexturePtr); | |
| // Free WASM memory in reverse allocation order (LIFO) | |
| wasmModule.exports.freeMemory(linkRangesTexturePtr); | |
| wasmModule.exports.freeMemory(linksTexturePtr); | |
| wasmModule.exports.freeMemory(positionsPtr); | |
| wasmModule.exports.freeMemory(linksDataPtr); | |
| wasmModule.exports.freeMemory(nodesDataPtr); |
| // For fragments inside the circle, offset depth proportionally | ||
| if (r <= 1.0) { | ||
| // Closer to edge = larger depth offset (appears further back) | ||
| // This creates a spherical depth profile | ||
| float depthOffset = (1.0 - r) * 0.0001; | ||
| gl_FragDepth = gl_FragCoord.z + depthOffset; | ||
| } else { | ||
| gl_FragDepth = gl_FragCoord.z; | ||
| } |
There was a problem hiding this comment.
This shader writes to gl_FragDepth, which will fail to compile in WebGL1 contexts unless EXT_frag_depth is enabled and gl_FragDepthEXT is used (or depth writes are avoided). Please guard/enable the correct extension or make depth writes conditional on WebGL2 support.
| // For fragments inside the circle, offset depth proportionally | |
| if (r <= 1.0) { | |
| // Closer to edge = larger depth offset (appears further back) | |
| // This creates a spherical depth profile | |
| float depthOffset = (1.0 - r) * 0.0001; | |
| gl_FragDepth = gl_FragCoord.z + depthOffset; | |
| } else { | |
| gl_FragDepth = gl_FragCoord.z; | |
| } | |
| // For fragments inside the circle, offset depth proportionally | |
| #if defined(GL_EXT_frag_depth) | |
| if (r <= 1.0) { | |
| // Closer to edge = larger depth offset (appears further back) | |
| // This creates a spherical depth profile | |
| float depthOffset = (1.0 - r) * 0.0001; | |
| gl_FragDepthEXT = gl_FragCoord.z + depthOffset; | |
| } else { | |
| gl_FragDepthEXT = gl_FragCoord.z; | |
| } | |
| #elif __VERSION__ >= 300 | |
| if (r <= 1.0) { | |
| // Closer to edge = larger depth offset (appears further back) | |
| // This creates a spherical depth profile | |
| float depthOffset = (1.0 - r) * 0.0001; | |
| gl_FragDepth = gl_FragCoord.z + depthOffset; | |
| } else { | |
| gl_FragDepth = gl_FragCoord.z; | |
| } | |
| #else | |
| // Fallback: do not modify fragment depth when custom depth writes | |
| // are not supported (e.g., WebGL1 without EXT_frag_depth). | |
| #endif |
| const linksTexturePtr = memoryOffset; | ||
| memoryOffset += linksTextureSize; | ||
| const linkRangesTexturePtr = memoryOffset; | ||
|
|
There was a problem hiding this comment.
This worker writes input/output buffers starting at memoryOffset = 0. With the updated WASM now using heap.alloc internally (e.g., in processLinks), manual allocations from 0 can overlap the WASM heap region and corrupt allocations/results. Use the module’s exported allocateMemory/freeMemory (like texture-worker.js) or otherwise ensure your buffers start above the heap base and do not overlap the heap allocator’s growth area.
| } | ||
|
|
||
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, alpha ); | ||
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, opacity ); |
There was a problem hiding this comment.
alpha is computed as opacity * t (where t includes circle AA and optionally texel.a), but the output color uses opacity instead of alpha. This effectively ignores antialiasing and texture alpha (except for the discard threshold). Use the computed alpha in the final fragment color.
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, opacity ); | |
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, alpha ); |
| // Calculate custom depth to fix z-fighting with transparent points | ||
| // For fragments inside the circle, offset depth proportionally | ||
| if (r <= 1.0) { | ||
| // Closer to edge = larger depth offset (appears further back) | ||
| // This creates a spherical depth profile | ||
| float depthOffset = (1.0 - r) * 0.0001; | ||
| gl_FragDepth = gl_FragCoord.z + depthOffset; | ||
| } else { | ||
| gl_FragDepth = gl_FragCoord.z; | ||
| } |
There was a problem hiding this comment.
This shader writes to gl_FragDepth, which will fail to compile in WebGL1 contexts unless EXT_frag_depth is enabled and gl_FragDepthEXT is used (or depth writes are avoided). Please guard/enable the correct extension or make depth writes conditional on WebGL2 support.
| } | ||
|
|
||
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, alpha ); | ||
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, opacity ); |
There was a problem hiding this comment.
alpha is computed as opacity * t (where t includes circle AA and optionally texel.a), but the output color uses opacity instead of alpha. This effectively ignores antialiasing and texture alpha (except for the discard threshold). Use the computed alpha in the final fragment color.
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, opacity ); | |
| gl_FragColor = vec4( layer * mix( vec3( 1.0 ), vColor, inheritColors ) * uColor, alpha ); |
| const packedLinkAmount = wasmModule.exports.processTextures( | ||
| nodesDataPtr, | ||
| nodes.length, | ||
| linksDataPtr, | ||
| links.length, | ||
| textureSize, | ||
| positionsPtr, | ||
| linksTexturePtr, | ||
| linkRangesTexturePtr, | ||
| frustumSize | ||
| ); | ||
|
|
||
| if (packedLinkAmount < 0) { | ||
| throw new Error('Packed links exceed texture capacity'); | ||
| } |
There was a problem hiding this comment.
If an error is thrown after allocating WASM buffers (e.g., when packedLinkAmount < 0), the allocated pointers are never freed. This can permanently grow the WASM heap for the worker. Wrap allocation/processing/extraction in a try/finally (or similar) that always frees any successfully allocated pointers, and ensure the frees happen in the correct (reverse) order for the current allocator.
|
Claude finished @jonobr1's task —— View job Comprehensive PR Review: WASM Link Range & Packing ImplementationI've completed a thorough review of this PR that moves link range and packed link texture generation into WASM. Here's my detailed analysis: Todo List:
🟢 Significant Improvements Since Last Review✅ Memory Management Fixed:
✅ WASM Implementation Quality:
✅ Robust Architecture:
🟡 Remaining Critical Issues1. Memory Deallocation Order (Critical)Issue in heap.free(cursorsPtr); // ❌ Wrong order
heap.free(startOffsetsPtr); // ❌ Wrong order
heap.free(degreeCountsPtr); // ❌ Wrong orderProblem: AssemblyScript's stub heap uses LIFO (Last-In-First-Out) deallocation. Freeing in allocation order means only the last allocation gets reclaimed, causing memory growth over time. Fix Required: Reverse the deallocation order: heap.free(cursorsPtr); // ✅ Last allocated, freed first
heap.free(startOffsetsPtr); // ✅ Middle allocation
heap.free(degreeCountsPtr); // ✅ First allocated, freed lastImpact: This affects all workers ( 2. Missing Input ValidationBuffer overflow potential in multiple locations:
Recommendation: // Add before WASM calls:
if (nodes.length * 4 * 4 > MAX_SAFE_SIZE) {
throw new Error(`Input too large: ${nodes.length} nodes`);
}3. Incomplete Error CleanupResource leak potential in } catch (error) {
// ❌ Memory allocated but never freed on error paths
self.postMessage({ /* error response */ });
}Fix: Wrap in try-finally or use consistent cleanup patterns to ensure memory is always freed. 🔍 Security & Performance AssessmentSecurity: Generally Good ✅
Performance: Excellent Optimization 🚀
Shader Compatibility: Improved ✅The shader updates in
However, the final 🧪 Test Coverage GapsMissing test scenarios that should be added:
📊 Architecture Assessment: ExcellentThe three-tier processing strategy is well-implemented:
Worker Pipeline Integration: The updates to consume 🏁 Final RecommendationThis is a high-quality performance enhancement with excellent architecture. The critical memory management issues from the previous review have been successfully addressed. The remaining issues are primarily around:
Priority Actions:
The performance benefits and elegant degradation strategy make this a valuable addition. With the deallocation order fixed, this implementation will be robust and production-ready. Overall Assessment: ✅ Approve with critical memory management fix required |
|
Claude finished @jonobr1's task —— View job Comprehensive PR Review: WASM Link Range & Packing ImplementationI've completed a thorough review of this PR that moves link range and packed link texture generation into WASM. Here's my detailed analysis: Todo List:
🟢 Significant Improvements Since Previous Reviews✅ Memory Management Fixed:
✅ WASM Implementation Quality:
✅ Robust Architecture:
🔴 Critical Issues Found1. Memory Deallocation Order (Critical)Issue in heap.free(cursorsPtr); // ❌ Wrong order
heap.free(startOffsetsPtr); // ❌ Wrong order
heap.free(degreeCountsPtr); // ❌ Wrong orderProblem: AssemblyScript's stub heap uses LIFO (Last-In-First-Out) deallocation. Freeing in allocation order means only the last allocation gets reclaimed, causing memory growth over time. Fix Required: Reverse the deallocation order: heap.free(cursorsPtr); // ✅ Last allocated, freed first
heap.free(startOffsetsPtr); // ✅ Middle allocation
heap.free(degreeCountsPtr); // ✅ First allocated, freed lastImpact: This affects all worker memory management and will cause memory leaks in long-running applications. 2. Worker Memory Deallocation OrderSame LIFO issue in workers:
The workers should free in reverse allocation order (last allocated first). 🟡 Important Issues3. Input Validation GapsBuffer overflow potential in multiple locations:
Recommendation: // Add before WASM calls:
if (nodes.length * 4 * 4 > MAX_SAFE_SIZE) {
throw new Error(`Input too large: ${nodes.length} nodes`);
}4. Error Path Memory CleanupResource leak potential in workers: 🟢 Excellent ImplementationsShader Compatibility: Fixed ✅The shader updates in
The final Security: Generally Secure ✅
Performance: Excellent Optimization 🚀
Architecture: Well-DesignedThe three-tier processing strategy is excellent:
Worker Pipeline Integration: The updates to consume 🧪 Test Coverage AssessmentMissing test scenarios that should be added:
🏁 Final RecommendationThis is a high-quality performance enhancement with excellent architecture. The critical memory management issues from the previous review have been mostly addressed, but the LIFO deallocation order issue is critical for long-term stability. Priority Actions:
The performance benefits and elegant degradation strategy make this a valuable addition. With the deallocation order fixed, this implementation will be robust and production-ready. Overall Assessment: ✅ Approve with critical memory management fix required |
|
Claude finished @jonobr1's task —— View job Comprehensive PR Review: WASM Link Range & Packing ImplementationTodo List:
🟢 Significant Improvements Since Previous Reviews✅ Memory Management Fixed:
✅ WASM Implementation Quality:
✅ Robust Architecture:
🔴 Critical Issues Found1. Memory Deallocation Order (Critical)Issue in heap.free(cursorsPtr); // ❌ Wrong order
heap.free(startOffsetsPtr); // ❌ Wrong order
heap.free(degreeCountsPtr); // ❌ Wrong orderProblem: AssemblyScript's stub heap uses LIFO (Last-In-First-Out) deallocation. Freeing in allocation order means only the last allocation gets reclaimed, causing memory growth over time. Fix Required: Reverse the deallocation order: heap.free(cursorsPtr); // ✅ Last allocated, freed first
heap.free(startOffsetsPtr); // ✅ Middle allocation
heap.free(degreeCountsPtr); // ✅ First allocated, freed lastImpact: This affects all worker memory management and will cause memory leaks in long-running applications. 2. Worker Memory Deallocation OrderSame LIFO issue in workers:
The workers should free in reverse allocation order (last allocated first). 🟡 Important Issues3. Input Validation GapsBuffer overflow potential in multiple locations:
Recommendation: // Add before WASM calls:
if (nodes.length * 4 * 4 > MAX_SAFE_SIZE) {
throw new Error(`Input too large: ${nodes.length} nodes`);
}4. Error Path Memory CleanupResource leak potential in workers: 🟢 Excellent ImplementationsShader Compatibility: Fixed ✅The shader updates in
The final Security: Generally Secure ✅
Performance: Excellent Optimization 🚀
Architecture: Well-DesignedThe three-tier processing strategy is excellent:
Worker Pipeline Integration: The updates to consume 🧪 Test Coverage AssessmentMissing test scenarios that should be added:
🏁 Final RecommendationThis is a high-quality performance enhancement with excellent architecture. The critical memory management issues from the previous review have been mostly addressed, but the LIFO deallocation order issue is critical for long-term stability. Priority Actions:
The performance benefits and elegant degradation strategy make this a valuable addition. With the deallocation order fixed, this implementation will be robust and production-ready. Overall Assessment: ✅ Approve with critical memory management fix required |
|
Addressed on branch :\n\n- LIFO deallocation order is fixed in WASM link packing:\n - \n - \n- Worker allocation/free ordering and error-path cleanup are fixed with + reverse frees:\n - and \n - and \n - and \n- Broader validation and typed error handling added (//):\n - \n - \n - \n- Regression tests added and passing (
RUN v4.0.18 /Users/jonobrandel/Documents/force-directed-graph ✓ src/workers/tests/texture-worker.unit.test.js (5 tests) 2ms Test Files 2 passed (2) |
|
Correction to my previous note (shell escaping ate markdown): Addressed on branch codex/wasm-link-range-packing:
|
Summary
Files