FirelyTeam / firely-cql-sdk

BSD 3-Clause "New" or "Revised" License
30 stars 17 forks source link

Performance update for 1.0 #590

Closed DamienMosier closed 1 month ago

DamienMosier commented 1 month ago

Current 1.0

ADD-E: Duration: 3h 32m 35s CRE: Duration: 2h 46m 1s PND-E: Duration: 10h 48m 39s SPD: Duration: 3h 10m 21s

Time to run 80 HEDIS measures with multiple configurations using current develop 1.0 branch image

With these updates, same build pipeline

ADD-E: Duration: 38m 0s CRE: Duration: 57m 4s PND-E: Duration: 53m 39s SPD: Duration: 1h 36m 22s

image

ewoutkramer commented 1 month ago

@baseTwo - is this something we need in your equivalent 2.0 port as well?

baseTwo commented 1 month ago

@baseTwo - is this something we need in your equivalent 2.0 port as well?

We have an issue to add memoization as part of the context https://github.com/FirelyTeam/firely-cql-sdk/issues/600, however, it looks like 1.0 changed back to the old way where most libraries contain state for the context and the Lazy<>'s.

@DamienMosier , which measures do not have context defined, I'd like to compare the two types, but GitHub collapses too many files for me to go through each one until I find it.

DamienMosier commented 1 month ago

@ewoutkramer this is something you would definitely want in the 2.0 branch for the performance gains.

@baseTwo The way the logic work is if the file or one of the includes has a context then it creates it the old way with a constructor and utilizes Lazy. If the file does not, aka a library, then it's a singleton.

For our use case only our actual measures have context Patient and all of our libraries are either functions or fluent functions with the resources passed in. In your Demo project, context is added to almost all files which is why it looks like the old way.

The main performance issue with the original 1.0 branch was with the constructors. If you have LibraryA that has 5 includes and each of those subsequent libraries each have their own set of includes. You could end up instantiating the same library multiple times. Having it scoped allowed us to use Lazy so we gained the benefit of some caching but at the expense of calling the constructors a lot. Switching to singleton removed that constructor lift but removed the ability to use Lazy. I thought about it and realized we only need Lazy where there is context and all subsequent libraries did not need it and we could have the best of both worlds.

Here are some partial examples of measure AAB from DQIC if you want to see the CQL.

[System.CodeDom.Compiler.GeneratedCode(".NET Code Generation", "1.0.0.0")]
[CqlLibrary("AAB_Concepts", "2024.1.0")]
public class AAB_Concepts_2024_1_0
{

    public static AAB_Concepts_2024_1_0 Instance { get; }  = new();

    #region Dependencies
    #endregion

    [CqlDeclaration("Acute Bronchitis")]
    [CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1016")]
    public CqlValueSet Acute_Bronchitis(CqlContext context) => 
        new CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1016", null);

    [CqlDeclaration("Competing Diagnosis")]
    [CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1067")]
    public CqlValueSet Competing_Diagnosis(CqlContext context) => 
        new CqlValueSet("https://www.ncqa.org/fhir/valueset/2.16.840.1.113883.3.464.1004.1067", null);

and the actual file with context is reverted to the old way, the same way you see files in the DQIC repository.

[System.CodeDom.Compiler.GeneratedCode(".NET Code Generation", "1.0.0.0")]
[CqlLibrary("AAB_Reporting", "2024.1.0")]
public class AAB_Reporting_2024_1_0
{
    internal CqlContext context;

    #region Cached values

    internal Lazy<Tuples.Tuple_BOFQdLfCiMegGCeQZaSIJRaC> __Measure;
    internal Lazy<Patient> __Patient;
    internal Lazy<IEnumerable<ExplanationOfBenefit>> __Eobs;
    internal Lazy<IEnumerable<ExplanationOfBenefit>> __Admin_Eobs;

    #endregion
    public AAB_Reporting_2024_1_0(CqlContext context)
    {
        this.context = context ?? throw new ArgumentNullException("context");

        __Measure = new Lazy<Tuples.Tuple_BOFQdLfCiMegGCeQZaSIJRaC>(this.Measure_Value(context));
        __Patient = new Lazy<Patient>(this.Patient_Value(context));
        __Eobs = new Lazy<IEnumerable<ExplanationOfBenefit>>(this.Eobs_Value(context));
        __Admin_Eobs = new Lazy<IEnumerable<ExplanationOfBenefit>>(this.Admin_Eobs_Value(context));
DamienMosier commented 1 month ago

For reference, I generated this for Evan after doing testing. The number are based off our build machine times. The "1.0 DCS" is the original 1.0 prior to singleton. The "1.0 Develop" is current develop with only singleton's and the final are these changes that are pending.

image