DYN-1722 #9578
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -822,6 +822,16 @@ private FunctionEndPoint GetLooseCompliantFEP( | |
| return compliantTarget; | ||
| } | ||
|
|
||
| private Boolean IsCompatibleReplicationOption(List<ReplicationInstruction> oldOption, List<ReplicationInstruction> newOption) | ||
| { | ||
| if (oldOption.Count > 0 && newOption.Count > 0 && oldOption.Count < newOption.Count) | ||
| { | ||
| if (oldOption[0].ToString().Equals(newOption[0].ToString())) | ||
reddyashish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private void ComputeFeps( | ||
| Context context, | ||
| List<StackValue> arguments, | ||
|
|
@@ -832,6 +842,10 @@ private void ComputeFeps( | |
| out List<FunctionEndPoint> resolvesFeps, | ||
reddyashish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| out List<ReplicationInstruction> replicationInstructions) | ||
| { | ||
| replicationInstructions = null; | ||
| resolvesFeps = null; | ||
| Boolean flag = false; | ||
reddyashish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| #region Case 1: Replication guide with exact match | ||
| { | ||
| FunctionEndPoint fep = GetCompleteMatchFunctionEndPoint(context, arguments, funcGroup, instructions, stackFrame, runtimeCore); | ||
|
|
@@ -855,13 +869,17 @@ private void ComputeFeps( | |
| HashSet<FunctionEndPoint> lookups; | ||
| if (funcGroup.CanGetExactMatchStatics(context, reducedParams, stackFrame, runtimeCore, out lookups)) | ||
| { | ||
| if (replicationInstructions == null || IsCompatibleReplicationOption(replicationInstructions, replicationOption)) | ||
| { | ||
| //Otherwise we have a cluster of FEPs that can be used to dispatch the array | ||
|
There was a problem hiding this comment. I find this comment confusing - what is the |
||
| resolvesFeps = new List<FunctionEndPoint>(lookups); | ||
| replicationInstructions = replicationOption; | ||
| flag = true; | ||
| } | ||
| } | ||
| } | ||
| if (flag == true) | ||
| return; | ||
| } | ||
| #endregion | ||
|
|
||
|
|
@@ -888,9 +906,11 @@ private void ComputeFeps( | |
| { | ||
| resolvesFeps = new List<FunctionEndPoint>() { compliantTarget }; | ||
| replicationInstructions = replicationOption; | ||
| flag = true; | ||
| } | ||
| } | ||
| if (flag == true) | ||
| return; | ||
|
There was a problem hiding this comment. @reddyashish so - what about case 5 - why do we exit the other cases late, and exit case 5 as soon as we find a match? There was a problem hiding this comment. I did not understand this case and it adds an empty replication option to the previous list and finds the match. I was not able to find any examples related to this case. There was a problem hiding this comment. I will try to find when it's called. There was a problem hiding this comment. @mjkkirschner @reddyashish In general this whole function is still black magic to me. I have been able to look at indivual examples and how they flow through but it is hard to make judgment on individual PR's without holistic picture. I think we need to document examples of different data, data structures, replication settings, and functions and what the result replication instruction and function endpoint list. Not sure if we need to do that on this PR but it is hard to validate the code changes otherwise There was a problem hiding this comment. For a start - I've filed a followup task to cover each of the missing cases with explicit tests. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -923,9 +943,11 @@ private void ComputeFeps( | |
| { | ||
| resolvesFeps = new List<FunctionEndPoint>() { compliantTarget }; | ||
| replicationInstructions = replicationOption; | ||
| flag = true; | ||
| } | ||
| } | ||
| if (flag == true) | ||
| return; | ||
| } | ||
| #endregion | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure I understand this change. Why are you checking equality only for the first option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those 2 cases where it was failing, as we are checking for all the replication options(to find a match), the first match is found for "cartesian:indices=0" option. Then it finds a match for "zipped:indices=0,1" and it applies zipped replication option(as this is the final option that is tested). Previously, the first option that found a match was applied directly. After this change, it would not accept the zipped replication as the cartesian option has already found the match.
Another case is, we want to accept this new replication option, if it is of the higher rank than the previous option (old option: "cartesian:indices=0" and new option: "cartesian:indices=0, cartesian:indices=0". Similar option but checks for one extra depth level). Since all options are unique, I was checking the first element and the count for the new option to be higher than the first. Also can use, newOption.Count = oldOption.Count +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely having a hard time understanding this one, is it possible @reddyashish you could draw a diagram or do a longer writeup of the problem here and the approach taken to fix it - I know it's kind of a tall order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so - I'm trying to think of cases where this will fail -
I think naming this method or adding a summary might help - but my interpretation is that this method is used to find
List<ReplicationInstruction>where the first instruction matches the previous one but is of a greater depth?What I cannot figure out or explain yet is what case this is used to avoid or to guarantee we hit? Can you try to sum it up in the summary of the method or in the git here.