Skip to content

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

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Engine/ProtoCore/CodeGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,10 @@ protected void GenerateCallsiteIdentifierForGraphNode(AssociativeGraph.GraphNode
// Build the unique ID for a callsite
string callsiteIdentifier =
procName +
"_InClassDecl" + globalClassIndex +
"_InFunctionScope" + globalProcIndex +
"_Instance" + functionCallInstance.ToString() +
"_" + graphNode.guid.ToString();
Constants.kSingleUnderscore + Constants.kInClassDecl + globalClassIndex +
Constants.kSingleUnderscore + Constants.kInFunctionScope + globalProcIndex +
Constants.kSingleUnderscore + Constants.kInstance + functionCallInstance +
Constants.kSingleUnderscore + graphNode.guid;

// TODO Jun: Address this in MAGN-3774
// The current limitation is retrieving the cached trace data for multiple callsites in a single CBN
Expand Down
4 changes: 4 additions & 0 deletions src/Engine/ProtoCore/DSASM/Defs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,10 @@ public struct Constants
public const string kDoubleUnderscores = "__";
public const string kSingleUnderscore = "_";
public const string kTempVarForTypedIdentifier = "%tTypedIdent";

internal const string kInClassDecl = "InClassDecl";
internal const string kInFunctionScope = "InFunctionScope";
internal const string kInstance = "Instance";
}

public enum MemoryRegion
Expand Down
46 changes: 19 additions & 27 deletions src/Engine/ProtoCore/Lang/CallSite.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,12 @@ public bool HasAnyNestedData

if (HasData)
return true;
else
{
//Not empty, and doesn't have data so test recursive
Validity.Assert(NestedData != null,
"Invalid recursion logic, this is a VM bug, please report to the Dynamo Team");

//Not empty, and doesn't have data so test recursive
Validity.Assert(NestedData != null,
"Invalid recursion logic, this is a VM bug, please report to the Dynamo Team");

return NestedData.Any(srtd => srtd.HasAnyNestedData);
}
return NestedData.Any(srtd => srtd.HasAnyNestedData);
}
}

Expand Down Expand Up @@ -172,26 +170,21 @@ public ISerializable GetLeftMostData()
{
if (HasData)
return Data;
else
{
if (!HasNestedData)
return null;
else
{

if (!HasNestedData)
return null;
#if DEBUG

Validity.Assert(NestedData != null, "Nested data has changed null status since last check, suspected race");
Validity.Assert(NestedData.Count > 0, "Empty subnested array, please file repo data with @lukechurch, relates to MAGN-4059");
Validity.Assert(NestedData != null, "Nested data has changed null status since last check, suspected race");
Validity.Assert(NestedData.Count > 0, "Empty subnested array, please file repo data with @lukechurch, relates to MAGN-4059");
#endif

//Safety trap to protect against an empty array, need repro test to figure out why this is getting set with empty arrays
if (NestedData.Count == 0)
return null;
//Safety trap to protect against an empty array, need repro test to figure out why this is getting set with empty arrays
if (NestedData.Count == 0)
return null;

SingleRunTraceData nestedTraceData = NestedData[0];
return nestedTraceData.GetLeftMostData();
}
}
SingleRunTraceData nestedTraceData = NestedData[0];
return nestedTraceData.GetLeftMostData();
}

public List<SingleRunTraceData> NestedData;
Expand Down Expand Up @@ -238,7 +231,7 @@ public List<ISerializable> RecursiveGetNestedData()

/// <summary>
/// TraceBinder is used to find assemblies to be used for
/// deserialization in cases where the exact assemlby that was
/// deserialization in cases where the exact assembly that was
/// used in the serialization is not available.
/// </summary>
internal class TraceBinder : SerializationBinder
Expand Down Expand Up @@ -286,7 +279,7 @@ public override System.Type BindToType(string assemblyName, string typeName)
/// Normal usage patten is:
/// 1. Instantiate
/// 2. Push Trace data from callsite
/// 3. Call GetObjectData to serialise it onto a stream
/// 3. Call GetObjectData to serialize it onto a stream
/// 4. Recreate using the special constructor
/// </summary>
[Serializable]
Expand Down Expand Up @@ -325,14 +318,13 @@ public TraceSerialiserHelper(SerializationInfo info, StreamingContext context)
#if DEBUG
Debug.WriteLine("Deserialization of trace data failed.");
#endif
continue;
}
}

}

/// <summary>
/// Save the data into the standard serialisation pattern
/// Save the data into the standard serialization pattern
/// </summary>
public void GetObjectData(SerializationInfo info, StreamingContext context)
{
Expand Down Expand Up @@ -615,7 +607,7 @@ public bool WillCallReplicate(Context context, List<StackValue> arguments,

/// <summary>
/// Call this method to obtain the Base64 encoded string that
/// represent this instance of CallSite;s trace data
/// represents this callsite instance's trace data
/// </summary>
/// <returns>Returns the Base64 encoded string that represents the
/// trace data of this callsite
Expand Down
45 changes: 40 additions & 5 deletions src/Engine/ProtoCore/RuntimeData.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using ProtoCore.AssociativeGraph;
using ProtoCore.DSASM;
using ProtoCore.Utils;
Expand All @@ -25,11 +26,23 @@ public class RuntimeData


private Dictionary<Guid, List<CallSite.RawTraceData>> uiNodeToSerializedDataMap = null;

private static readonly string identifierPattern = @"([a-zA-Z-_@0-9]+)";
private static readonly string indexPattern = @"([-+]?\d+)";
private static readonly string callsiteIDPattern =
identifierPattern +
Constants.kInClassDecl + indexPattern +
Constants.kSingleUnderscore + Constants.kInFunctionScope + indexPattern +
Constants.kSingleUnderscore + Constants.kInstance + indexPattern +
identifierPattern;
private static readonly string joinPattern = ';' + callsiteIDPattern;
private static readonly string fullCallsiteID = callsiteIDPattern + string.Format("({0})*", joinPattern);

public IDictionary<string, CallSite> CallsiteCache { get; set; }
/// <summary>
/// Map from a callsite's guid to a graph UI node.
/// </summary>
public Dictionary<Guid, Guid> CallSiteToNodeMap { get; private set; }
public Dictionary<Guid, Guid> CallSiteToNodeMap { get; }

#endregion

Expand Down Expand Up @@ -289,7 +302,7 @@ public void SetTraceDataForNodes(

/// <summary>
/// Call this method to pop the top-most serialized callsite trace data.
/// Note that this call only pops off a signle callsite trace data
/// Note that this call only pops off a single callsite trace data
/// belonging to a given UI node denoted by the given node guid.
/// </summary>
/// <param name="nodeGuid">The Guid of a given UI node whose top-most
Expand Down Expand Up @@ -320,7 +333,7 @@ private string GetAndRemoveTraceDataForNode(Guid nodeGuid, string callsiteID)
{
for (int i = 0; i < callsiteDataList.Count; i++)
{
if (callsiteDataList[i].ID == callsiteID)
if (DoCallSiteIDsMatch(callsiteID, callsiteDataList[i].ID))
{
callsiteTraceData = callsiteDataList[i].Data;
callsiteDataList.RemoveAt(i);
Expand All @@ -344,10 +357,32 @@ private string GetAndRemoveTraceDataForNode(Guid nodeGuid, string callsiteID)
{
return GetAndRemoveTraceDataForNode(nodeGuid, string.Empty);
}
else

return callsiteTraceData;
}

private static bool DoCallSiteIDsMatch(string compilerGenerated, string deserialized)
{
if (compilerGenerated == deserialized) return true;

var matches1 = Regex.Match(compilerGenerated, fullCallsiteID);
var matches2 = Regex.Match(deserialized, fullCallsiteID);

if (matches1.Groups.Count != matches2.Groups.Count) return false;

// 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.

// that can vary for custom nodes or DS functions that make nested calls to
// host element creation methods.
for (int i = 0; i < matches1.Groups.Count; i++)
{
return callsiteTraceData;
if (i == 0 || i == 6 || i == 9) continue;

if (matches1.Groups[i].Value != matches2.Groups[i].Value) return false;
}

return true;
}

#endregion // Trace Data Serialization Methods/Members
Expand Down