Skip to content

Conversation

@jasonstratton
Copy link
Contributor

Fix #DYN-8717

Bug: Geometry inside a Dictionary gets incorrectly garbage collected when a downstream node's output type changes, even though the Dictionary still holds a valid reference.

Changes:

  • Added RecursiveMarkCLRContainer() to Heap.cs for traversing CLR collections, arrays, and class fields
  • Added test coverage in CLRContainerGCTests.cs with 11 test cases
  • Created GC performance benchmark tool in tools/Performance/GCBenchmark

Purpose

#DYN-8717 - Graph error causes subsequent evaluation to fail

Root Cause: The DesignScript GC cannot trace references stored inside CLR objects. Dictionary values are stored in ImmutableDictionary<string, object> (CLR), not in DSObject.Values (DS heap).

Extended DesignScript garbage collector to trace references stored inside CLR objects (Dictionary, List, etc.). Previously, the GC could only trace references in DSObject.Values, causing premature collection of geometry stored in Dictionaries when downstream nodes changed types.

Declarations

Check these if you believe they are true

Release Notes

Prevention of geometry objects stored inside CLR containers (like Dictionary) from geting prematurely garbage collected when a downstream node's output type changes, even though the container still holds a valid reference to the geometry.

Performance Notes

Measured by running benchmarks before and after the fix using the performance graphs.

The overall performance profile shows a slight net improvement (-3.9% average), with targeted overhead only in the scenarios that require CLR container traversal.

The implementation adds some overhead to scenarios involving CLR container traversal (12-18% slower), which is expected.

Extended DesignScript garbage collector to trace references stored
inside CLR objects (Dictionary, List, etc.). Previously, the GC could
only trace references in DSObject.Values, causing premature collection
of geometry stored in Dictionaries when downstream nodes changed types.

Changes:
- Added RecursiveMarkCLRContainer() to Heap.cs for traversing CLR
  collections, arrays, and class fields
- Added test coverage in CLRContainerGCTests.cs with 11 test cases
- Created GC performance benchmark tool in tools/Performance/GCBenchmark

Fix #DYN-8717

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 9, 2026 16:01
@github-actions github-actions bot changed the title Fix GC premature collection of objects in CLR containers DYN-8717: Fix GC premature collection of objects in CLR containers Jan 9, 2026
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8717

@jasonstratton jasonstratton requested review from a team and benglin January 9, 2026 16:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical bug (DYN-8717) where geometry objects stored inside CLR containers like Dictionary were prematurely garbage collected. The root cause was that DesignScript's garbage collector could only trace references in DSObject.Values, missing references stored in CLR-backed containers.

Changes:

  • Extended the GC to traverse CLR containers (Dictionary, List, etc.) to find nested DS references
  • Added comprehensive test coverage for CLR container scenarios
  • Created a performance benchmark tool to measure GC impact

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/Performance/GCBenchmark/README.md Documentation for new GC benchmark tool with usage instructions and test descriptions
tools/Performance/GCBenchmark/Program.cs Command-line interface and result formatting for benchmark tool
tools/Performance/GCBenchmark/GCBenchmarkRunner.cs Benchmark test implementations exercising various GC scenarios
tools/Performance/GCBenchmark/GCBenchmark.csproj Project configuration for benchmark tool
tools/Performance/GCBenchmark/BenchmarkResult.cs Data structure for storing benchmark metrics
test/Engine/ProtoTest/DSASM/CLRContainerGCTests.cs 11 test cases validating CLR container GC behavior
src/Engine/ProtoCore/FFI/CLRObjectMarshaler.cs Methods to extract nested DS references from CLR containers
src/Engine/ProtoCore/DSASM/Heap.cs Modified GC mark phase to traverse CLR containers
Comments suppressed due to low confidence (1)

test/Engine/ProtoTest/DSASM/CLRContainerGCTests.cs:1

  • The complex boolean expression with 9 conditions is difficult to read and maintain. Consider breaking it into multiple assertions or intermediate variables for better readability and easier debugging when tests fail.
using System;

}
}
// Handle DesignScript.Builtin.Dictionary specifically - access its internal D field
else if (obj.GetType().FullName == "DesignScript.Builtin.Dictionary")
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic string 'DesignScript.Builtin.Dictionary' should be extracted as a named constant to improve maintainability. For example: private const string DS_DICTIONARY_TYPE = \"DesignScript.Builtin.Dictionary\";

Copilot uses AI. Check for mistakes.
QilongTang and others added 4 commits January 12, 2026 16:15
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@RobertGlobant20
Copy link
Contributor

@jasonstratton Correct me if I'm wrong but in the original graph the Data.ParseJSON (Dictionary) was creating the Geometry in the first step but in the second step when changing the boolean to False the Output of the Code Block is assigned to be a string (so the Geometry generated by Dictionary is disposed), in the third step the boolean is set to True again and the Geometry is not created again (was disposed previously) so this was generating the error and due that the Data.ParseJSON node is not executed again is not created again.
I understand that your fix will be avoiding to dispose the Geometry due that Data.ParseJSON is still referencing it, right?,
then if I update something before the Data.ParseJSON node (like the json path or if is a string some value) then the previous Geometry will be disposed and a new one will be created?

@jasonstratton
Copy link
Contributor Author

I understand that your fix will be avoiding to dispose the Geometry due that Data.ParseJSON is still referencing it, right?, then if I update something before the Data.ParseJSON node (like the json path or if is a string some value) then the previous Geometry will be disposed and a new one will be created?

@RobertGlobant20, very interesting point. I'm not sure what would happen. I will need to test it. I think what will happen is that it would behave as it did before my change, which should be that everything that is recalculated in the graph is disposed.

@RobertGlobant20
Copy link
Contributor

I understand that your fix will be avoiding to dispose the Geometry due that Data.ParseJSON is still referencing it, right?, then if I update something before the Data.ParseJSON node (like the json path or if is a string some value) then the previous Geometry will be disposed and a new one will be created?

@RobertGlobant20, very interesting point. I'm not sure what would happen. I will need to test it. I think what will happen is that it would behave as it did before my change, which should be that everything that is recalculated in the graph is disposed.

I will try to spend some time in testing this changes, my biggest concern is that modifying the GC the normal behavior could be affected but let's test to see if everything is working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants