You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The callsite ID and trace data pair serialized to a DYN file are read (if they exist for a node) and stored into TLS only if the ID that's stored in the file matches with the one generated by the compiler for the same node GUID. However if there is a custom node that makes a call to a nested element creation node, the callsite ID generated does not work well.
This is because the ID is built from a combination of several parameters including the function scope of the creation node itself. In the case of the custom node, the custom node itself is a function in the global scope however the node that it calls into has a non-global function scope, which is the function index of the custom node in the global function table.
Basing the callsite identifier on something like a function index in the global function table can be very flaky as it could change depending on the order in which global functions (like custom nodes) are compiled and loaded into the table. As a result of this a bug was found where on loading a graph from an older version of Dynamo into a newer version caused element binding to break. This was because the callsite ID serialized to the file did not match with that generated by the compiler.
This PR attempts to fix this situation by ignoring the function scope of the nested method call in the custom node while comparing the callsite ID's.
This change fixes one of the issues with element binding with custom nodes. The replication issue is yet to be addressed.
The reason will be displayed to describe this comment to others. Learn more.
I guess my reason for wanting to understand this better is to be able to ask more specific questions like - without these 3 groups, can we sufficiently identify these trace data entries from each other and definitely associate them with the correct nested node?
The reason will be displayed to describe this comment to others. Learn more.
@mjkkirschner had some time to answer you today. See the following example for a callsite identifier for a custom node:
In this example, the deserialized and compiler generated ID's match in every respect except for the function scope of the nested node. The function scope generated by an older version of Dynamo, which is serialized in the old DYN file is 110, while the function index of the same custom node when loaded in the latest Dynamo version (compiler generated) is 107.
If you see the function groups, there are 12 of them broken down as shown. You can see that the 1st, 7th and 10th groups have the function scope number in them (107 in this example), which need to be ignored. This is based on the way that the Regex (funcCallsiteID) is generated.
To answer your other question - does this ID comparison still ensure uniqueness in indentification of custom node instances ; Yes, it still does. Say if there is another nested node called inside the same custom node, even though its function scope will be the same, it can be uniquely identified by its name, 8th group or its node guid, 12th group.
The reason will be displayed to describe this comment to others. Learn more.
@aparajit-pratap thanks for following up! 😄
the team will look at finishing this one up ASAP.
aparajit-pratap
changed the title
[DNM] DYN-1642: Callsite identifier changes to fix element binding issues with custom nodes
[WIP] DYN-1642: Callsite identifier changes to fix element binding issues with custom nodes
Mar 13, 2019
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The callsite ID and trace data pair serialized to a DYN file are read (if they exist for a node) and stored into TLS only if the ID that's stored in the file matches with the one generated by the compiler for the same node GUID. However if there is a custom node that makes a call to a nested element creation node, the callsite ID generated does not work well.
This is because the ID is built from a combination of several parameters including the function scope of the creation node itself. In the case of the custom node, the custom node itself is a function in the global scope however the node that it calls into has a non-global function scope, which is the function index of the custom node in the global function table.
Basing the callsite identifier on something like a function index in the global function table can be very flaky as it could change depending on the order in which global functions (like custom nodes) are compiled and loaded into the table. As a result of this a bug was found where on loading a graph from an older version of Dynamo into a newer version caused element binding to break. This was because the callsite ID serialized to the file did not match with that generated by the compiler.
This PR attempts to fix this situation by ignoring the function scope of the nested method call in the custom node while comparing the callsite ID's.
This change fixes one of the issues with element binding with custom nodes. The replication issue is yet to be addressed.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
FYIs
@QilongTang @mjkkirschner