Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] DYN-1642: Callsite identifier changes to fix element binding issues with custom nodes #9527

Closed
wants to merge 2 commits into from

Conversation

aparajit-pratap
Copy link
Contributor

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

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

FYIs

@QilongTang @mjkkirschner


// If both group counts match, they should number 12 in all.
// We should ignore checking for the 1st, 7th, and 10th group specifically
// as per the Regex pattern (for fullCallsiteID) since that group includes the function scope
Copy link
Member

@mjkkirschner mjkkirschner Mar 9, 2019

Choose a reason for hiding this comment

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

can you provide a sample here -
I'm having a hard time understanding what the 1st,7th,or 10th entry refer to?

Copy link
Member

Choose a reason for hiding this comment

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?

Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Mar 21, 2019

Choose a reason for hiding this comment

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:
image

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.

Copy link
Member

Choose a reason for hiding this comment

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 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
@mjkkirschner mjkkirschner self-assigned this Mar 28, 2019
@aparajit-pratap aparajit-pratap deleted the dyn-1642 branch April 5, 2019 20:02
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.

2 participants