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.

Sign up for GitHub

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

Jump to bottom

Avoid inefficient DataRetriever calls #44

Merged
merged 5 commits into from
Sep 12, 2023
Merged

Avoid inefficient DataRetriever calls #44

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4ec202
Avoid repeating GetProperty for each retrieve.
ewoutkramer Aug 15, 2023
9e2f1ab
Made sure GetProperty is not generated when the default codepath is u…
ewoutkramer Aug 15, 2023
37239bb
Merge branch 'remove-agileobjects' into avoid-inefficient-retrieve-calls
ewoutkramer Sep 5, 2023
cedac43
Regenerated the C#
ewoutkramer Sep 5, 2023
02bf335
Merge branch 'develop' into avoid-inefficient-retrieve-calls
ewoutkramer Sep 12, 2023
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
12 changes: 0 additions & 12 deletions Cql/CodeGeneration.NET/CSharpSourceCodeWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,18 +710,6 @@ private string ToCode(int indent, Expression expression, bool leadingIndent = tr
return code;
}
}
else if (constant.Type == typeof(PropertyInfo))
{
if (constant.Value != null && constant.Value is PropertyInfo propertyInfo)
{
var declaringType = PrettyTypeName(propertyInfo.DeclaringType ??
throw new InvalidOperationException($"PropertyInfo.DeclaringType is null"));
var code = $"{leadingIndentString}typeof({declaringType}).GetProperty(\"{propertyInfo.Name}\")";
return code;
}
else
return $"{leadingIndentString}null";
}
else if (constant.Type == typeof(string))
{
if (constant.Value == null)
Expand Down
43 changes: 6 additions & 37 deletions Cql/CodeGeneration.NET/Visitors/BlockTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,13 @@ public BlockTransformer(VariableNameGenerator nameGenerator, IEnumerable<string>
UseLazyBools = useLazyBools;
}

public override Expression? Visit(Expression? node)
{
if (node == null)
return node;
switch (node.NodeType)
public override Expression? Visit(Expression? node) =>
node switch
{
case ExpressionType.Call:
case ExpressionType.NewArrayInit:
case ExpressionType.MemberInit:
case ExpressionType.Lambda:
return base.Visit(node);
default:
return node;
}
}

protected override Expression VisitMethodCall(MethodCallExpression node)
{
return ToBlock(node);

}
{ NodeType: ExpressionType.Call or ExpressionType.NewArrayInit or ExpressionType.MemberInit } =>
ToBlock(node),
_ => node,
};

private Expression ToBlock(Expression node)
{
Expand Down Expand Up @@ -112,22 +98,5 @@ private Expression ToBlock(Expression node)
// else return Expression.Block(@return);
//}

protected override Expression VisitMemberInit(MemberInitExpression node)
{
return ToBlock(node);
}

protected override Expression VisitNewArray(NewArrayExpression node)
{
return ToBlock(node);
}

protected override Expression VisitLambda<T>(Expression<T> node)
{
var newBody = Visit(node.Body) ??
throw new InvalidOperationException("Visit returned null");
var newLambda = Expression.Lambda(newBody, node.Parameters);
return newLambda;
}
}
}
39 changes: 23 additions & 16 deletions Cql/CodeGeneration.NET/Visitors/ExtractLocalVariablesTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,25 +127,32 @@ protected override Expression VisitMemberInit(MemberInitExpression node)

protected override Expression VisitNewArray(NewArrayExpression node)
{
// replaces
// var x = new T[] { exprA, exprB, exprC }};
// with
// var p = exprA;
// var q = exprB;
// var r = exprC;
// var x = new T[] { p,q,r };

if (node.NodeType == ExpressionType.NewArrayInit)
{
var newExpressions = new Expression[node.Expressions.Count];
for (int i = 0; i < node.Expressions.Count; i++)
{
var expression = node.Expressions[i];
var vn = NameGenerator.Next();
var newLocal = Expression.Parameter(expression.Type, vn);
var visitedExpression = Visit(expression);
var localAssignment = Expression.Assign(newLocal, visitedExpression);
Locals.Add(localAssignment);
newExpressions[i] = newLocal;
}
var elementType = node.Type.GetElementType()
?? throw new InvalidOperationException($"Unable to determine array element type for {node.Type.FullName}");
var newInit = Expression.NewArrayInit(elementType, newExpressions);
return newInit;
var newLocalVariables = node.Expressions.Select(makeLocalAssignment);
var elementType = node.Type.GetElementType()!;
return Expression.NewArrayInit(elementType, newLocalVariables);
}
else
return node;

ParameterExpression makeLocalAssignment(Expression valueExpr)
{
var vn = NameGenerator.Next();
var newLocal = Expression.Parameter(valueExpr.Type, vn);
var visitedExpression = Visit(valueExpr);
var assignment = Expression.Assign(newLocal, visitedExpression);
Locals.Add(assignment); // note: side-effect on Locals
return newLocal;
}
else return node;
}


Expand Down
12 changes: 0 additions & 12 deletions Cql/CodeGeneration.NET/Visitors/LocalVariableDeduper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,5 @@ protected override Expression VisitBlock(BlockExpression node)
}
return newBody;
}

protected override Expression VisitLambda<T>(Expression<T> node)
{
if (node is LambdaExpression lambda)
{
var newLambdaBody = Visit(lambda.Body)
?? throw new InvalidOperationException("Visit returned null");
var newLambda = Expression.Lambda(newLambdaBody, lambda.Parameters);
return newLambda;
}
return base.VisitLambda(node);
}
}
}
1 change: 1 addition & 0 deletions Cql/Cql.Compiler/Cql.Compiler.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

<ItemGroup>
<ProjectReference Include="..\Cql.Logging\Cql.Logging.csproj" />
<ProjectReference Include="..\Cql.Model\Cql.Model.csproj" />
<ProjectReference Include="..\Cql.Operators\Cql.Operators.csproj" />
<ProjectReference Include="..\Cql.ValueSets\Cql.ValueSets.csproj" />
<ProjectReference Include="..\Cql.Runtime\Cql.Runtime.csproj" />
Expand Down
9 changes: 9 additions & 0 deletions Cql/Cql.Compiler/CqlOperatorsBinding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,15 @@ private MethodCallExpression Retrieve(MemberExpression operators,
&& codePropertyExpression is ConstantExpression cpe
&& cpe.Type == typeof(PropertyInfo))
{
if (cpe.Value is PropertyInfo pi)
{
var declaringType = pi!.DeclaringType;
var propName = pi.Name;
var method = typeof(Type).GetMethod(nameof(Type.GetProperty), new[] { typeof(string) })!;
var typeOf = Expression.Constant(declaringType);
codePropertyExpression = Expression.Call(typeOf, method, Expression.Constant(propName));
}

return Retrieve(operators, type, valueSetOrCodes, codePropertyExpression);
}
else throw new ArgumentException("Second parameter to Retrieve is expected to be a constant PropertyInfo", nameof(codePropertyExpression));
Expand Down
23 changes: 19 additions & 4 deletions Cql/Cql.Compiler/ExpressionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

using Hl7.Cql.Elm;
using Hl7.Cql.Model;
using Hl7.Cql.Primitives;
using Hl7.Cql.Runtime;
using Hl7.Cql.ValueSets;
Expand Down Expand Up @@ -2038,30 +2039,44 @@ protected LambdaExpression WithToSelectManyBody(Type tupleType,
return selectManyLambda;
}

// Yeah, hardwired to FHIR 4.0.1 for now.
private static readonly IDictionary<string, ClassInfo> modelMapping = Models.ClassesById(Models.Fhir401);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make the ModelInfo an abstract property on TypeResolver and require all implementations to provide one. ExpressionBuilder can refer to it through its TypeResolver. Theoretically, we need 1 TypeResolver per model declared in using statements in the provided library, e.g. to handle

using FHIR version '4.0.1'
using QICore version '1.0.0'

A single TypeResolver won't be enough for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot do this without a reorg of our project structure, since TypeResolver is in Abstractions, which cannot refer to Model, since Model already indirectly references Abstractions, so there's a circular reference.

However, I'd like to pull the PRs before we do a reorg, to avoid merge conflicts, so I have turned this suggestion into an issue to pick up after we've pulled the open PRs.

See #66.


protected Expression Retrieve(Retrieve retrieve, ExpressionBuilderContext ctx)
{
Type? sourceElementType;
string? cqlRetrieveResultType;

// SingletonFrom does not have this specified; in this case use DataType instead
if (retrieve.resultTypeSpecifier == null)
{
if (string.IsNullOrWhiteSpace(retrieve.dataType.Name))
throw new ArgumentException("If a Retrieve lacks a ResultTypeSpecifier it must have a DataType", nameof(retrieve));
var dataType = retrieve.dataType;
sourceElementType = TypeResolver.ResolveType(dataType.Name);
cqlRetrieveResultType = retrieve.dataType.Name;

sourceElementType = TypeResolver.ResolveType(cqlRetrieveResultType);
}
else
{
if (retrieve.resultTypeSpecifier is elm.ListTypeSpecifier listTypeSpecifier)
{
cqlRetrieveResultType = listTypeSpecifier.elementType is elm.NamedTypeSpecifier nts ? nts.name.Name : null;
sourceElementType = TypeManager.TypeFor(listTypeSpecifier.elementType, ctx);
}
else throw new NotImplementedException($"Sources with type {retrieve.resultTypeSpecifier.GetType().Name} are not implemented.");
}

Expression? codeProperty;
if (sourceElementType != null && retrieve.codeProperty != null)

var hasCodePropertySpecified = sourceElementType != null && retrieve.codeProperty != null;
var isDefaultCodeProperty = retrieve.codeProperty is null ||
(cqlRetrieveResultType is not null &&
modelMapping.TryGetValue(cqlRetrieveResultType, out ClassInfo? classInfo) &&
classInfo.primaryCodePath == retrieve.codeProperty);

if (hasCodePropertySpecified && !isDefaultCodeProperty)
{
var codePropertyInfo = TypeResolver.GetProperty(sourceElementType!, retrieve.codeProperty);
var codePropertyInfo = TypeResolver.GetProperty(sourceElementType!, retrieve.codeProperty!);
codeProperty = Expression.Constant(codePropertyInfo, typeof(PropertyInfo));
}
else
Expand Down
1 change: 1 addition & 0 deletions Cql/Cql.Model/Cql.Model.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
<ProjectReference Include="..\Cql.Runtime\Cql.Runtime.csproj" />
<PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.11.0-beta1.23364.2" PrivateAssets="All" />
<InternalsVisibleTo Include="CoreTests" Key="$(LibraryPKHash)" />
<InternalsVisibleTo Include="Hl7.Cql.Compiler" Key="$(LibraryPKHash)" />
</ItemGroup>

</Project>
10 changes: 5 additions & 5 deletions Demo/Measures/AdultOutpatientEncountersFHIR4-2.2.000.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,25 +125,25 @@ private IEnumerable<Encounter> Qualifying_Encounters_Value()
{
var a_ = this.Office_Visit();
var b_ = context?.Operators.RetrieveByValueSet<Encounter>(a_,
typeof(Encounter).GetProperty("Type"));
null);
var c_ = this.Annual_Wellness_Visit();
var d_ = context?.Operators.RetrieveByValueSet<Encounter>(c_,
typeof(Encounter).GetProperty("Type"));
null);
var e_ = context?.Operators.ListUnion<Encounter>(b_,
d_);
var f_ = this.Preventive_Care_Services___Established_Office_Visit__18_and_Up();
var g_ = context?.Operators.RetrieveByValueSet<Encounter>(f_,
typeof(Encounter).GetProperty("Type"));
null);
var h_ = this.Preventive_Care_Services_Initial_Office_Visit__18_and_Up();
var i_ = context?.Operators.RetrieveByValueSet<Encounter>(h_,
typeof(Encounter).GetProperty("Type"));
null);
var j_ = context?.Operators.ListUnion<Encounter>(g_,
i_);
var k_ = context?.Operators.ListUnion<Encounter>(e_,
j_);
var l_ = this.Home_Healthcare_Services();
var m_ = context?.Operators.RetrieveByValueSet<Encounter>(l_,
typeof(Encounter).GetProperty("Type"));
null);
var n_ = context?.Operators.ListUnion<Encounter>(k_,
m_);
Func<Encounter,bool?> w_ = (ValidEncounter) =>
Expand Down
30 changes: 15 additions & 15 deletions Demo/Measures/AdvancedIllnessandFrailtyExclusionECQMFHIR4-5.17.000.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ private IEnumerable<MedicationRequest> Dementia_Medications_In_Year_Before_or_Du
{
var a_ = this.Dementia_Medications();
var b_ = context?.Operators.RetrieveByValueSet<MedicationRequest>(a_,
typeof(MedicationRequest).GetProperty("Medication"));
null);
var e_ = context?.Operators.ListUnion<MedicationRequest>(b_,
b_);
Func<MedicationRequest,bool?> y_ = (DementiaMed) =>
Expand Down Expand Up @@ -291,10 +291,10 @@ private IEnumerable<CqlInterval<CqlDateTime>> Long_Term_Care_Periods_During_Meas
{
var a_ = this.Care_Services_in_Long_Term_Residential_Facility();
var b_ = context?.Operators.RetrieveByValueSet<Encounter>(a_,
typeof(Encounter).GetProperty("Type"));
null);
var c_ = this.Nursing_Facility_Visit();
var d_ = context?.Operators.RetrieveByValueSet<Encounter>(c_,
typeof(Encounter).GetProperty("Type"));
null);
var e_ = context?.Operators.ListUnion<Encounter>(b_,
d_);
Func<Encounter,bool?> n_ = (LongTermFacilityEncounter) =>
Expand Down Expand Up @@ -333,18 +333,18 @@ private IEnumerable<Encounter> Outpatient_Encounters_with_Advanced_Illness_Value
{
var a_ = this.Outpatient();
var b_ = context?.Operators.RetrieveByValueSet<Encounter>(a_,
typeof(Encounter).GetProperty("Type"));
null);
var c_ = this.Observation();
var d_ = context?.Operators.RetrieveByValueSet<Encounter>(c_,
typeof(Encounter).GetProperty("Type"));
null);
var e_ = context?.Operators.ListUnion<Encounter>(b_,
d_);
var f_ = this.Emergency_Department_Visit();
var g_ = context?.Operators.RetrieveByValueSet<Encounter>(f_,
typeof(Encounter).GetProperty("Type"));
null);
var h_ = this.Nonacute_Inpatient();
var i_ = context?.Operators.RetrieveByValueSet<Encounter>(h_,
typeof(Encounter).GetProperty("Type"));
null);
var j_ = context?.Operators.ListUnion<Encounter>(g_,
i_);
var k_ = context?.Operators.ListUnion<Encounter>(e_,
Expand All @@ -363,7 +363,7 @@ private IEnumerable<Encounter> Outpatient_Encounters_with_Advanced_Illness_Value
{
var q_ = this.Advanced_Illness();
var r_ = context?.Operators.RetrieveByValueSet<Condition>(q_,
typeof(Condition).GetProperty("Code"));
null);
Func<Condition,bool?> aj_ = (AdvancedIllnessDiagnosis) =>
{
var s_ = MATGlobalCommonFunctionsFHIR4_6_1_000.EncounterDiagnosis(OutpatientEncounter);
Expand Down Expand Up @@ -544,7 +544,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
{
var a_ = this.Acute_Inpatient();
var b_ = context?.Operators.RetrieveByValueSet<Encounter>(a_,
typeof(Encounter).GetProperty("Type"));
null);
Func<Encounter,bool?> f_ = (AcuteInpatient) =>
{
var d_ = (AcuteInpatient?.StatusElement as object);
Expand All @@ -559,7 +559,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
{
var h_ = this.Advanced_Illness();
var i_ = context?.Operators.RetrieveByValueSet<Condition>(h_,
typeof(Condition).GetProperty("Code"));
null);
Func<Condition,bool?> aa_ = (AdvancedIllnessDiagnosis) =>
{
var j_ = MATGlobalCommonFunctionsFHIR4_6_1_000.EncounterDiagnosis(InpatientEncounter);
Expand Down Expand Up @@ -604,7 +604,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
{
var a_ = this.Frailty_Device();
var b_ = context?.Operators.RetrieveByValueSet<DeviceRequest>(a_,
typeof(DeviceRequest).GetProperty("Code"));
null);
var e_ = context?.Operators.ListUnion<DeviceRequest>(b_,
b_);
Func<DeviceRequest,bool?> v_ = (FrailtyDeviceOrder) =>
Expand Down Expand Up @@ -642,7 +642,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
v_);
var x_ = context?.Operators.ExistsInList<DeviceRequest>(w_);
var z_ = context?.Operators.RetrieveByValueSet<Observation>(a_,
typeof(Observation).GetProperty("Code"));
null);
Func<Observation,bool?> al_ = (FrailtyDeviceApplied) =>
{
var aa_ = (FrailtyDeviceApplied?.StatusElement as object);
Expand Down Expand Up @@ -674,7 +674,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
an_);
var ap_ = this.Frailty_Diagnosis();
var aq_ = context?.Operators.RetrieveByValueSet<Condition>(ap_,
typeof(Condition).GetProperty("Code"));
null);
Func<Condition,bool?> at_ = (FrailtyDiagnosis) =>
{
var ar_ = MATGlobalCommonFunctionsFHIR4_6_1_000.Prevalence_Period(FrailtyDiagnosis);
Expand All @@ -690,7 +690,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
av_);
var ax_ = this.Frailty_Encounter();
var ay_ = context?.Operators.RetrieveByValueSet<Encounter>(ax_,
typeof(Encounter).GetProperty("Type"));
null);
Func<Encounter,bool?> bh_ = (FrailtyEncounter) =>
{
var ba_ = (FrailtyEncounter?.StatusElement as object);
Expand All @@ -714,7 +714,7 @@ private IEnumerable<Encounter> Inpatient_Encounter_with_Advanced_Illness_Value()
bj_);
var bl_ = this.Frailty_Symptom();
var bm_ = context?.Operators.RetrieveByValueSet<Observation>(bl_,
typeof(Observation).GetProperty("Code"));
null);
Func<Observation,bool?> bz_ = (FrailtySymptom) =>
{
var bn_ = (FrailtySymptom?.StatusElement as object);
Expand Down
Loading